New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How does CEReactions interact with thrown exceptions? #3217

Closed
bzbarsky opened this Issue Nov 9, 2017 · 11 comments

Comments

5 participants
@bzbarsky
Collaborator

bzbarsky commented Nov 9, 2017

The spec says:

Operations, attributes, setters, or deleters annotated with the [CEReactions] extended attribute must run the following steps surrounding the actions listed in the description of the operation, setter, deleter, or the attribute's setter:

etc. What does that mean in cases when the actions throw an exception. Is the exception caught, the CEReactions run, then the exception rethrown? Do the CEReactions run while JS execution is in some bizarre "in the middle of trying to throw an exception" state? Do the "After executing the listed actions" bits not run?

A naive reading would suggest that the "After executing the listed actions" bits don't run, but that would fail to pop the element queue.

What Firefox apparently implemented is "CEReactions run while JS execution is in some bizarre state", which is bogus and we're going to need to change that.

Note that this situation can be triggered if the algorithm steps have side-effects that happen before throwing. The testcase that triggered the problem in Firefox uses something along the lines of:

document.documentElement.before('', document.documentElement);

which inserts a textnode and then throws...

@domenic @annevk

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Nov 9, 2017

Member

The intent is to rethrow, if I recall. I will clarify this.

Member

domenic commented Nov 9, 2017

The intent is to rethrow, if I recall. I will clarify this.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 9, 2017

Member

I thought we settled on report the exception, similar to events. As the callbacks are invoked the moment just before you return from IDL, at which point it doesn't make sense for the algorithm to throw.

Member

annevk commented Nov 9, 2017

I thought we settled on report the exception, similar to events. As the callbacks are invoked the moment just before you return from IDL, at which point it doesn't make sense for the algorithm to throw.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Nov 9, 2017

Collaborator

I thought we settled on report the exception, similar to events.

For exceptions thrown by the reaction callbacks, yes.

This issue is about exceptions thrown from the IDL actions defined for the setter or operation.

So in my before() example above, the sequence is as follows:

  1. before is called.
  2. A textnode is inserted into the DOM, possibly queues some reaction callbacks.
  3. A attempt is made to insert an element where it can't be inserted, throws an exception.
  4. What should happen here is the question. Do the reaction callbacks queued at step 2 run? Does the exception thrown in step 3 propagate out to the caller of before? (Imo it should!)
Collaborator

bzbarsky commented Nov 9, 2017

I thought we settled on report the exception, similar to events.

For exceptions thrown by the reaction callbacks, yes.

This issue is about exceptions thrown from the IDL actions defined for the setter or operation.

So in my before() example above, the sequence is as follows:

  1. before is called.
  2. A textnode is inserted into the DOM, possibly queues some reaction callbacks.
  3. A attempt is made to insert an element where it can't be inserted, throws an exception.
  4. What should happen here is the question. Do the reaction callbacks queued at step 2 run? Does the exception thrown in step 3 propagate out to the caller of before? (Imo it should!)
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Nov 10, 2017

Member

Yeah, report an exception is not right; that would essentially mean adding [CEReactions] to APIs causes them to stop throwing exceptions despite their spec text.

My thinking is that it should essentially do

try {
  runAlgorithm();
} finally {
  runReactions(); // never throws, only reports exceptions
}

That's what I intend to spec when I have a spare moment, although TPAC makes that a bit tricky.

Member

domenic commented Nov 10, 2017

Yeah, report an exception is not right; that would essentially mean adding [CEReactions] to APIs causes them to stop throwing exceptions despite their spec text.

My thinking is that it should essentially do

try {
  runAlgorithm();
} finally {
  runReactions(); // never throws, only reports exceptions
}

That's what I intend to spec when I have a spare moment, although TPAC makes that a bit tricky.

@snuggs

This comment has been minimized.

Show comment
Hide comment
@snuggs

snuggs Nov 10, 2017

Member

TPAC makes that a bit tricky - @domenic

Assuming because TPAC is still underway and topics being discussed?

Member

snuggs commented Nov 10, 2017

TPAC makes that a bit tricky - @domenic

Assuming because TPAC is still underway and topics being discussed?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk
Member

annevk commented Nov 10, 2017

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 10, 2017

Member

@domenic hmm, how many algorithms do we have that queue reactions and then throw? Range operations?

Member

annevk commented Nov 10, 2017

@domenic hmm, how many algorithms do we have that queue reactions and then throw? Range operations?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Nov 10, 2017

Collaborator

@bzbarsky per https://dom.spec.whatwg.org/#dom-childnode-before it should throw before step 2.

Why?

Any of the DOM append/insert methods can queue and then throw when passed a document fragment, I would think....

Collaborator

bzbarsky commented Nov 10, 2017

@bzbarsky per https://dom.spec.whatwg.org/#dom-childnode-before it should throw before step 2.

Why?

Any of the DOM append/insert methods can queue and then throw when passed a document fragment, I would think....

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Nov 10, 2017

Collaborator

Hmm, sorry, that's not right.

So I need to look carefully through the fuzzer testcase that caught this in Firefox to see what's throwing and what's queuing the callback. It's at view-source:https://bug1415761.bmoattachments.org/attachment.cgi?id=8926677 for what it's worth.

Collaborator

bzbarsky commented Nov 10, 2017

Hmm, sorry, that's not right.

So I need to look carefully through the fuzzer testcase that caught this in Firefox to see what's throwing and what's queuing the callback. It's at view-source:https://bug1415761.bmoattachments.org/attachment.cgi?id=8926677 for what it's worth.

@EdgarChen

This comment has been minimized.

Show comment
Hide comment
@EdgarChen

EdgarChen Nov 11, 2017

The fuzzer testcase in https://bugzilla.mozilla.org/show_bug.cgi?id=1415761 is something like,

customElements.define('custom-element', class extends HTMLElement {
  disconnectedCallback() {}
});

var text = document.createTextNode('');
document.documentElement.appendChild(text);

var element = document.createElement('custom-element');
document.documentElement.appendChild(element);

text.before('', document.documentElement);

The sequence of before operation is as follow,

  1. Convert '' and document.documentElement. (Step 4 of https://dom.spec.whatwg.org/#dom-childnode-before)
  2. A new TextNode is created. (Step 2 of https://dom.spec.whatwg.org/#converting-nodes-into-a-node)
  3. Append TextNode and document.documentElement to a new created DocumentFragment. (Step 4 of https://dom.spec.whatwg.org/#converting-nodes-into-a-node)
    3.1.) Adopt document.documentElement. (Step 4 of https://dom.spec.whatwg.org/#concept-node-pre-insert)
    3.2.) Remove document.documentElement from its parent. (Step 2 of https://dom.spec.whatwg.org/#concept-node-adopt)
    3.3.) Enqueue disconnectedCallback for element. (Step 15.2 of https://dom.spec.whatwg.org/#concept-node-remove)
    3.4.) ...
  4. Insert the DocumentFragment. (Step 6 of https://dom.spec.whatwg.org/#dom-childnode-before)
    4.1.) Run pre-insertion validity checks. (Step 1 of https://dom.spec.whatwg.org/#concept-node-pre-insert)
    4.2.) Since the text's parent is document.documentElement and document.documentElement's parent now is DocumentFragment, throw HierarchyRequestError exception in Step 2 of https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity.
    ....

So 3.3.) enqueues the callback and 4.2) throws the excpetion.

EdgarChen commented Nov 11, 2017

The fuzzer testcase in https://bugzilla.mozilla.org/show_bug.cgi?id=1415761 is something like,

customElements.define('custom-element', class extends HTMLElement {
  disconnectedCallback() {}
});

var text = document.createTextNode('');
document.documentElement.appendChild(text);

var element = document.createElement('custom-element');
document.documentElement.appendChild(element);

text.before('', document.documentElement);

The sequence of before operation is as follow,

  1. Convert '' and document.documentElement. (Step 4 of https://dom.spec.whatwg.org/#dom-childnode-before)
  2. A new TextNode is created. (Step 2 of https://dom.spec.whatwg.org/#converting-nodes-into-a-node)
  3. Append TextNode and document.documentElement to a new created DocumentFragment. (Step 4 of https://dom.spec.whatwg.org/#converting-nodes-into-a-node)
    3.1.) Adopt document.documentElement. (Step 4 of https://dom.spec.whatwg.org/#concept-node-pre-insert)
    3.2.) Remove document.documentElement from its parent. (Step 2 of https://dom.spec.whatwg.org/#concept-node-adopt)
    3.3.) Enqueue disconnectedCallback for element. (Step 15.2 of https://dom.spec.whatwg.org/#concept-node-remove)
    3.4.) ...
  4. Insert the DocumentFragment. (Step 6 of https://dom.spec.whatwg.org/#dom-childnode-before)
    4.1.) Run pre-insertion validity checks. (Step 1 of https://dom.spec.whatwg.org/#concept-node-pre-insert)
    4.2.) Since the text's parent is document.documentElement and document.documentElement's parent now is DocumentFragment, throw HierarchyRequestError exception in Step 2 of https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity.
    ....

So 3.3.) enqueues the callback and 4.2) throws the excpetion.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 11, 2017

Member

Ah okay, so there are side effects before throwing. That's unfortunate, but I guess that's how it is. I think that's also the case for range APIs, but I haven't verified. #3217 (comment) seems reasonable.

Member

annevk commented Nov 11, 2017

Ah okay, so there are side effects before throwing. That's unfortunate, but I guess that's how it is. I think that's also the case for range APIs, but I haven't verified. #3217 (comment) seems reasonable.

domenic added a commit that referenced this issue Nov 17, 2017

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 21, 2017

Bug 1415761 - Catch the exception and rethrow it after invoking custo…
…m elements reactions; r=bz

The spec was unclear on how CEReactions interact with thrown exceptions; see whatwg/html#3217.
The spec is now being clarified in whatwg/html#3235.

MozReview-Commit-ID: 521HprTRS7k

--HG--
extra : rebase_source : 107d331203d0d16062fa061569e822d3c6d5f2c9

xeonchen pushed a commit to xeonchen/gecko that referenced this issue Nov 22, 2017

Bug 1415761 - Catch the exception and rethrow it after invoking custo…
…m elements reactions; r=bz

The spec was unclear on how CEReactions interact with thrown exceptions; see whatwg/html#3217.
The spec is now being clarified in whatwg/html#3235.

MozReview-Commit-ID: 521HprTRS7k

aethanyc pushed a commit to aethanyc/gecko-dev that referenced this issue Nov 22, 2017

Bug 1415761 - Catch the exception and rethrow it after invoking custo…
…m elements reactions; r=bz

The spec was unclear on how CEReactions interact with thrown exceptions; see whatwg/html#3217.
The spec is now being clarified in whatwg/html#3235.

MozReview-Commit-ID: 521HprTRS7k

JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Nov 28, 2017

Bug 1415761 - Catch the exception and rethrow it after invoking custo…
…m elements reactions; r=bz

The spec was unclear on how CEReactions interact with thrown exceptions; see whatwg/html#3217.
The spec is now being clarified in whatwg/html#3235.

MozReview-Commit-ID: 521HprTRS7k

jgraham added a commit to web-platform-tests/wpt that referenced this issue Nov 28, 2017

Catch the exception and rethrow it after invoking custom elements rea…
…ctions;

The spec was unclear on how CEReactions interact with thrown exceptions; see whatwg/html#3217.
The spec is now being clarified in whatwg/html#3235.

MozReview-Commit-ID: 521HprTRS7k

Upstreamed from https://bugzilla.mozilla.org/show_bug.cgi?id=1415761 [ci skip]

jgraham pushed a commit to jgraham/web-platform-tests that referenced this issue Dec 4, 2017

wpt-sync
Catch the exception and rethrow it after invoking custom elements rea…
…ctions;

The spec was unclear on how CEReactions interact with thrown exceptions; see whatwg/html#3217.
The spec is now being clarified in whatwg/html#3235.
bugzilla-url: https://bugzilla-dev.allizom.org/show_bug.cgi?id=Bug 1415761
gecko-commit: df3695afb47e0598e3fb1425a7e2a0d16adf35ee
gecko-integration-branch: central
gecko-reviewers: bz

jgraham added a commit to jgraham/web-platform-tests that referenced this issue Dec 4, 2017

Catch the exception and rethrow it after invoking custom elements rea…
…ctions;

The spec was unclear on how CEReactions interact with thrown exceptions; see whatwg/html#3217.
The spec is now being clarified in whatwg/html#3235.
bugzilla-url: https://bugzilla-dev.allizom.org/show_bug.cgi?id=Bug 1415761
gecko-commit: df3695afb47e0598e3fb1425a7e2a0d16adf35ee
gecko-integration-branch: central
gecko-reviewers: bz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment