187805 – Release assert when throwing exceptions in custom element reactions

RESOLVED FIXED 187805

Release assert when throwing exceptions in custom element reactions

https://bugs.webkit.org/show_bug.cgi?id=187805

Summary Release assert when throwing exceptions in custom element reactions

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	Details Formatted Diff Diff
Rebaselined bindings tests (16.26 KB, patch) 2018-08-02 22:25 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixed the change log (15.94 KB, patch) 2018-08-03 00:13 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (15.94 KB, patch) 2018-08-03 00:34 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Ryosuke Niwa

Reported

2018-07-19 09:13 PDT

Modified

2018-08-03 16:22 PDT History

CC List

12 users Show

URL

https://html.spec.whatwg.org/multipage/custom-elements.html#cereactions

Keywords InRadar

Duplicates (1)

179805 View as bug list

Depends on

188277

Blocks

154907

Dependencies

tree graph