createElement should not transmit exceptions but report them.

createElement should not transmit exceptions but report them.

BUG=652157, 658066
Committed: https://crrev.com/a21493ce8e12e552cd51c85523d451357bd427e1
Cr-Commit-Position: refs/heads/master@{#427306}

[dominicc (has gone to gerrit), 2016-10-21 08:34:30]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 08:34:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-10-21 08:35:40]

dominicc@chromium.org changed reviewers:
+ kojii@chromium.org

[dominicc (has gone to gerrit), 2016-10-21 08:37:03]

PTAL

Our tests could use some clean-up but for now I'm just trying to update them to
the new behavior.

[commit-bot: I haz the power, 2016-10-21 10:03:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 10:03:50]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-21 14:37:39]

non-owner lgtm. Great to have less code paths, thanks to the spec change!

[haraken, 2016-10-23 22:49:19]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-10-23 22:49:20]

https://codereview.chromium.org/2443543002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2443543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:163:
HTMLElement* ScriptCustomElementDefinition::handleCreateElementSyncException(

Do you know why the spec requires to throw the exception synchronously? This
looks like a very special logic to the custom element. (See how hacky the
implementation of V8ScriptRunner::throwException is.)

https://codereview.chromium.org/2443543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:180:
ScriptState::Scope scope(m_scriptState.get());

Don't you need to check:

  if (!m_scriptState->contextIsValid())
    return;

?

[dominicc (has gone to gerrit), 2016-10-24 01:14:29]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 01:14:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-10-24 01:15:06]

PTAL

Fixed the context check, not sure what else is needed so put some context in
replies inline.

https://codereview.chromium.org/2443543002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2443543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:163:
HTMLElement* ScriptCustomElementDefinition::handleCreateElementSyncException(
On 2016/10/23 at 22:49:20, haraken wrote:
> Do you know why the spec requires to throw the exception synchronously? This
looks like a very special logic to the custom element. (See how hacky the
implementation of V8ScriptRunner::throwException is.)

What in particular is the problem?

Re: the spec--The spec changed and now requires us to report, not throw, the
exception. This method reports the exception with dispatchErrorEvent. rniwa
suggested this change so that synchronous element creation by the parser and
synchronous element creation by createElement reported exceptions in the same
way. I personally preferred to have createElement throw the exception, but not
strongly enough to get in the way of interoperable implementations. The
discussion is in the TPAC2016 Web Platform WG Monday minutes.

Re: V8ScriptRunner--It does look hacky. What does V8ScriptRunner::throwException
have to do with this?

https://codereview.chromium.org/2443543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:180:
ScriptState::Scope scope(m_scriptState.get());
On 2016/10/23 at 22:49:19, haraken wrote:
> Don't you need to check:
> 
>   if (!m_scriptState->contextIsValid())
>     return;
> 
> ?

Good catch, ty, fixed.

[kojii, 2016-10-24 02:40:20]

On 2016/10/24 at 01:15:06, dominicc wrote:
> Re: V8ScriptRunner--It does look hacky. What does
V8ScriptRunner::throwException have to do with this?

The parser creating custom elements synchronously is exceptional in that there's
no JS callers on the JS stack. Normally, if we call V8ThrowException, it'll be
pending/scheduled when the next JS runs, but when there's no JS callers, it'll
be thrown away.

throwStackOverflowExceptionIfNeeded
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
is another exceptional case that does very similar thing (throws without JS
caller, but does not set ScriptOrigin.)

Since we're merging the code path for parser and document.createElement, if we
can distinguish it's called from document.createElement, we can use
V8ThrowException for it. One subtle difference is that, V8ThrowException will
report the error occurred in the caller (i.e., createElement) while
V8ScriptRunner::throwException reports it occurred in the constructor. It
affects file/lineno and whether the error should be muted or not. I don't have
strong opinion which is better as long as one of these are reported.

Last I heard, Anne and Domenic are working on rewriting "report exception" spec.
It'll be "note to mute by default", so muting issue (when we don't set
ScriptOrigin) is likely to be gone sooner or later, but this part is likely to
be undefined as per https://github.com/w3c/webcomponents/issues/547

[commit-bot: I haz the power, 2016-10-24 02:51:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-24 02:51:46]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-10-24 05:26:32]

On 2016/10/24 02:40:20, kojii wrote:
> On 2016/10/24 at 01:15:06, dominicc wrote:
> > Re: V8ScriptRunner--It does look hacky. What does
> V8ScriptRunner::throwException have to do with this?
> 
> The parser creating custom elements synchronously is exceptional in that
there's
> no JS callers on the JS stack. Normally, if we call V8ThrowException, it'll be
> pending/scheduled when the next JS runs, but when there's no JS callers, it'll
> be thrown away.
> 
> throwStackOverflowExceptionIfNeeded
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> is another exceptional case that does very similar thing (throws without JS
> caller, but does not set ScriptOrigin.)
> 
> Since we're merging the code path for parser and document.createElement, if we
> can distinguish it's called from document.createElement, we can use
> V8ThrowException for it. One subtle difference is that, V8ThrowException will
> report the error occurred in the caller (i.e., createElement) while
> V8ScriptRunner::throwException reports it occurred in the constructor. It
> affects file/lineno and whether the error should be muted or not. I don't have
> strong opinion which is better as long as one of these are reported.
> 
> Last I heard, Anne and Domenic are working on rewriting "report exception"
spec.
> It'll be "note to mute by default", so muting issue (when we don't set
> ScriptOrigin) is likely to be gone sooner or later, but this part is likely to
> be undefined as per https://github.com/w3c/webcomponents/issues/547

Thanks for the clarification. LGTM.

[dominicc (has gone to gerrit), 2016-10-25 04:12:00]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 04:12:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-10-25 04:30:26]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-10-25 04:30:26]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2443543002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-10-25 04:30:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 05:46:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 05:46:31]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dominicc (has gone to gerrit), 2016-10-25 06:49:47]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 06:50:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-10-25 07:23:23]

Description was changed from

==========
createElement should not transmit exceptions but report them.

BUG=652157
==========

to

==========
createElement should not transmit exceptions but report them.

BUG=652157,658066
==========

[commit-bot: I haz the power, 2016-10-25 08:23:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 08:23:36]

Dry run: This issue passed the CQ dry run.

[dominicc (has gone to gerrit), 2016-10-25 08:26:17]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-10-25 08:26:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2443543002/#ps80001
(title: "Rebaseline tests.")

[commit-bot: I haz the power, 2016-10-25 08:26:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 09:45:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-25 09:47:40]

Description was changed from

==========
createElement should not transmit exceptions but report them.

BUG=652157,658066
==========

to

==========
createElement should not transmit exceptions but report them.

BUG=652157,658066

Committed: https://crrev.com/a21493ce8e12e552cd51c85523d451357bd427e1
Cr-Commit-Position: refs/heads/master@{#427306}
==========

[commit-bot: I haz the power, 2016-10-25 09:47:41]

Patchset 5 (id:??) landed as
https://crrev.com/a21493ce8e12e552cd51c85523d451357bd427e1
Cr-Commit-Position: refs/heads/master@{#427306}