Bug 187805

Summary: Release assert when throwing exceptions in custom element reactions
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: DOMAssignee: 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 Flags
Fixes the bug
none
Rebaselined bindings tests
none
Fixed the change log
none
Patch for landing none

Frédéric Wang (:fredw)
Reported 2018-07-19 09:13:04 PDT
Crash test: https://w3c-test.org/custom-elements/reactions/with-exceptions.html #0 0x00007fdcc8648acc in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:267 #1 0x00007fdcd32f104a in JSC::ExceptionScope::assertNoException (this=0x7ffc0c172fa0) at DerivedSources/ForwardingHeaders/JavaScriptCore/ExceptionScope.h:46 #2 0x00007fdcc801517d in JSC::Interpreter::executeCall (this=0x7fdcb26ff7a8, callFrame=0x7fdc606ddfa8, function=0x7fdc4566a8b0, callType=<incomplete type>, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:973 #3 0x00007fdcc824763e in JSC::call (exec=0x7fdc606ddfa8, functionObject=..., callType=<incomplete type>, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/runtime/CallData.cpp:41 #4 0x00007fdcc82476fb in JSC::call (exec=0x7fdc606ddfa8, functionObject=..., callType=<incomplete type>, callData=..., thisValue=..., args=..., returnedException=...) at ../../Source/JavaScriptCore/runtime/CallData.cpp:48 #5 0x00007fdcd3c737d8 in (anonymous namespace)::JSMainThreadExecState::call (exec=0x7fdc606ddfa8, functionObject=..., callType=<incomplete type>, callData=..., thisValue=..., args=..., returnedException=...) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:54 #6 0x00007fdcd3c6fc67 in (anonymous namespace)::JSCustomElementInterface::invokeCallback((anonymous namespace)::Element &, JSC::JSObject *, const WTF::Function<void(JSC::ExecState*, WebCore::JSDOMGlobalObject*, JSC::MarkedArgumentBuffer&)> &) ( this=0x7fdc542a1630, element=..., callback=0x7fdc4566a8b0, addArguments=...) at ../../Source/WebCore/bindings/js/JSCustomElementInterface.cpp:254 #7 0x00007fdcd3c6fe6e in (anonymous namespace)::JSCustomElementInterface::invokeDisconnectedCallback (this=0x7fdc542a1630, element=...) at ../../Source/WebCore/bindings/js/JSCustomElementInterface.cpp:279 #8 0x00007fdcd3fc79c6 in (anonymous namespace)::CustomElementReactionQueueItem::invoke (this=0x7fdcb2660380, element=..., elementInterface=...) at ../../Source/WebCore/dom/CustomElementReactionQueue.cpp:82 #9 0x00007fdcd3fc3b25 in (anonymous namespace)::CustomElementReactionQueue::invokeAll (this=0x55d0007d83e0, element=...) at ../../Source/WebCore/dom/CustomElementReactionQueue.cpp:209 #10 0x00007fdcd3fc7c88 in (anonymous namespace)::CustomElementReactionStack::ElementQueue::invokeAll (this=0x55d000863a40) at ../../Source/WebCore/dom/CustomElementReactionQueue.cpp:230 #11 0x00007fdcd3fc3c6c in (anonymous namespace)::CustomElementReactionStack::processQueue (this=0x7ffc0c173500) at ../../Source/WebCore/dom/CustomElementReactionQueue.cpp:256 #12 0x00007fdcd2e232f3 in (anonymous namespace)::CustomElementReactionStack::~CustomElementReactionStack (this=0x7ffc0c173500, __in_chrg=<optimized out>) at ../../Source/WebCore/dom/CustomElementReactionQueue.h:74 #13 0x00007fdcd541da20 in (anonymous namespace)::jsCharacterDataPrototypeFunctionBeforeBody (state=0x7ffc0c173600, castedThis=0x7fdc60663c20, throwScope=...) at DerivedSources/WebCore/JSCharacterData.cpp:384 #14 0x00007fdcd5423818 in (anonymous namespace)::IDLOperation<WebCore::JSCharacterData>::call<WebCore::jsCharacterDataPrototypeFunctionBeforeBody> (state=..., operationName=0x7fdcd803d533 "before") at ../../Source/WebCore/bindings/js/JSDOMOperation.h:53 #15 0x00007fdcd541da49 in (anonymous namespace)::jsCharacterDataPrototypeFunctionBefore (state=0x7ffc0c173600) at DerivedSources/WebCore/JSCharacterData.cpp:394
Attachments
Fixes the bug (13.29 KB, patch)
2018-08-02 22:02 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Rebaselined bindings tests (16.26 KB, patch)
2018-08-02 22:25 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed the change log (15.94 KB, patch)
2018-08-03 00:13 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch for landing (15.94 KB, patch)
2018-08-03 00:34 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2018-07-20 02:25:16 PDT
*** Bug 179805 has been marked as a duplicate of this bug. ***
Radar WebKit Bug Importer
Comment 2 2018-07-20 09:44:21 PDT
<rdar://problem/42432714>
Ryosuke Niwa
Comment 3 2018-08-02 22:02:50 PDT
Created attachment 346454 [details] Fixes the bug
EWS Watchlist
Comment 4 2018-08-02 22:07:55 PDT
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
Ryosuke Niwa
Comment 5 2018-08-02 22:25:51 PDT
Created attachment 346455 [details] Rebaselined bindings tests
Frédéric Wang (:fredw)
Comment 6 2018-08-03 00:01:27 PDT
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.
Ryosuke Niwa
Comment 7 2018-08-03 00:13:16 PDT
Created attachment 346460 [details] Fixed the change log
Saam Barati
Comment 8 2018-08-03 00:25:14 PDT
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
Frédéric Wang (:fredw)
Comment 9 2018-08-03 00:26:13 PDT
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.
Ryosuke Niwa
Comment 10 2018-08-03 00:28:58 PDT
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.
Ryosuke Niwa
Comment 11 2018-08-03 00:29:49 PDT
(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.
Ryosuke Niwa
Comment 12 2018-08-03 00:34:15 PDT
Created attachment 346461 [details] Patch for landing
Ryosuke Niwa
Comment 13 2018-08-03 00:34:25 PDT
Comment on attachment 346461 [details] Patch for landing Wait for EWS.
Ryosuke Niwa
Comment 14 2018-08-03 01:16:50 PDT
Committed r234539: <https://trac.webkit.org/changeset/234539>
Yusuke Suzuki
Comment 15 2018-08-03 08:06:14 PDT
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?
Ryosuke Niwa
Comment 16 2018-08-03 16:13:26 PDT
(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.
Yusuke Suzuki
Comment 17 2018-08-03 16:22:49 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.