Bug 187805 - Release assert when throwing exceptions in custom element reactions
Summary: Release assert when throwing exceptions in custom element reactions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL: https://html.spec.whatwg.org/multipag...
Keywords: InRadar
: 179805 (view as bug list)
Depends on: 188277
Blocks: 154907
  Show dependency treegraph
 
Reported: 2018-07-19 09:13 PDT by Frédéric Wang (:fredw)
Modified: 2018-08-03 16:22 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 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
Comment 1 Frédéric Wang (:fredw) 2018-07-20 02:25:16 PDT
*** Bug 179805 has been marked as a duplicate of this bug. ***
Comment 2 Radar WebKit Bug Importer 2018-07-20 09:44:21 PDT
<rdar://problem/42432714>
Comment 3 Ryosuke Niwa 2018-08-02 22:02:50 PDT
Created attachment 346454 [details]
Fixes the bug
Comment 4 EWS Watchlist 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
Comment 5 Ryosuke Niwa 2018-08-02 22:25:51 PDT
Created attachment 346455 [details]
Rebaselined bindings tests
Comment 6 Frédéric Wang (:fredw) 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.
Comment 7 Ryosuke Niwa 2018-08-03 00:13:16 PDT
Created attachment 346460 [details]
Fixed the change log
Comment 8 Saam Barati 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
Comment 9 Frédéric Wang (:fredw) 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.
Comment 10 Ryosuke Niwa 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.
Comment 11 Ryosuke Niwa 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.
Comment 12 Ryosuke Niwa 2018-08-03 00:34:15 PDT
Created attachment 346461 [details]
Patch for landing
Comment 13 Ryosuke Niwa 2018-08-03 00:34:25 PDT
Comment on attachment 346461 [details]
Patch for landing

Wait for EWS.
Comment 14 Ryosuke Niwa 2018-08-03 01:16:50 PDT
Committed r234539: <https://trac.webkit.org/changeset/234539>
Comment 15 Yusuke Suzuki 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?
Comment 16 Ryosuke Niwa 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.
Comment 17 Yusuke Suzuki 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.