Summary: | Release assert when throwing exceptions in custom element reactions | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, dbates, d, esprehn+autocc, ews-watchlist, kangil.han, mark.lam, rniwa, rwlbuis, saam, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
URL: | https://html.spec.whatwg.org/multipage/custom-elements.html#cereactions | ||||||||||||
Bug Depends on: | 188277 | ||||||||||||
Bug Blocks: | 154907 | ||||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2018-07-19 09:13:04 PDT
*** Bug 179805 has been marked as a duplicate of this bug. *** Created attachment 346454 [details]
Fixes the bug
Comment on attachment 346454 [details] Fixes the bug Attachment 346454 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/8745999 New failing tests: (JS) JSTestCEReactions.cpp (JS) JSTestCEReactionsStringifier.cpp Created attachment 346455 [details]
Rebaselined bindings tests
Comment on attachment 346455 [details] Rebaselined bindings tests View in context: https://bugs.webkit.org/attachment.cgi?id=346455&action=review > LayoutTests/imported/w3c/ChangeLog:22 > + Duplicated changelog entry. Created attachment 346460 [details]
Fixed the change log
Comment on attachment 346460 [details] Fixed the change log View in context: https://bugs.webkit.org/attachment.cgi?id=346460&action=review > Source/WebCore/dom/CustomElementReactionQueue.cpp:251 > + invokeAll(); - Does this already clear any exception that may be thrown when running it? - If so, it can be moved out of the block Comment on attachment 346460 [details] Fixed the change log View in context: https://bugs.webkit.org/attachment.cgi?id=346460&action=review I'm not really familiar with the JSC API, but this LGTM from my understanding of the custom element spec. > Source/WebCore/dom/CustomElementReactionQueue.h:89 > + ALWAYS_INLINE CustomElementReactionStack(JSC::ExecState* state) Is the by-pointer version ever used elsewhere than to define the by-reference version? > LayoutTests/ChangeLog:3 > + Crash when throwing exceptions in custom element reactions And this and below you might want to change to the current bug title. Comment on attachment 346460 [details] Fixed the change log View in context: https://bugs.webkit.org/attachment.cgi?id=346460&action=review >> Source/WebCore/dom/CustomElementReactionQueue.cpp:251 >> + invokeAll(); > > - Does this already clear any exception that may be thrown when running it? > - If so, it can be moved out of the block Yes. Will do. (In reply to Frédéric Wang (:fredw) from comment #9) > Comment on attachment 346460 [details] > Fixed the change log > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346460&action=review > > I'm not really familiar with the JSC API, but this LGTM from my > understanding of the custom element spec. > > > Source/WebCore/dom/CustomElementReactionQueue.h:89 > > + ALWAYS_INLINE CustomElementReactionStack(JSC::ExecState* state) > > Is the by-pointer version ever used elsewhere than to define the > by-reference version? It's used in CustomElementReactionQueue::processBackupQueue() and JSMainThreadNullState. > > LayoutTests/ChangeLog:3 > > + Crash when throwing exceptions in custom element reactions > > And this and below you might want to change to the current bug title. Will fix. Created attachment 346461 [details]
Patch for landing
Comment on attachment 346461 [details]
Patch for landing
Wait for EWS.
Committed r234539: <https://trac.webkit.org/changeset/234539> Comment on attachment 346461 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=346461&action=review > Source/WebCore/bindings/js/JSMainThreadExecState.h:164 > explicit JSMainThreadNullState() > : m_previousState(JSMainThreadExecState::s_mainThreadState) > + , m_customElementReactionStack(m_previousState) Is this change necessary? (In reply to Yusuke Suzuki from comment #15) > Comment on attachment 346461 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346461&action=review > > > Source/WebCore/bindings/js/JSMainThreadExecState.h:164 > > explicit JSMainThreadNullState() > > : m_previousState(JSMainThreadExecState::s_mainThreadState) > > + , m_customElementReactionStack(m_previousState) > > Is this change necessary? Yes, if Objective-C DOM is used within a delegate callback while there is a JS exception in the stack, then we'd have to clear & rethrow that exception. Comment on attachment 346461 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=346461&action=review >>> Source/WebCore/bindings/js/JSMainThreadExecState.h:164 >>> + , m_customElementReactionStack(m_previousState) >> >> Is this change necessary? > > Yes, if Objective-C DOM is used within a delegate callback while there is a JS exception in the stack, then we'd have to clear & rethrow that exception. Ah, ignore me. I thought this field should be touched elsewhere, but I overlooked the constructor/destructor of m_customElementReactionStack have the side-effect. |