Bug 214774

Summary: Fixed exception check handling below ScheduledAction::executeFunctionInContext()
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: WebCore JavaScriptAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: mark.lam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
mark.lam: review+
Patch for Landing mark.lam: review+

Description Michael Saboff 2020-07-24 18:08:55 PDT
The simulated exception code found some issues in ScheduledAction::executeFunctionInContext() causing us not not properly account for exceptions thrown and caught there.

<rdar://problem/65925857>
Comment 1 Michael Saboff 2020-07-24 18:15:23 PDT
Created attachment 405209 [details]
Patch
Comment 2 Mark Lam 2020-07-24 18:25:21 PDT
Comment on attachment 405209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405209&action=review

r=me with fixes.

> Source/WebCore/bindings/js/ScheduledAction.cpp:122
> +    EXCEPTION_ASSERT(catchScope.exception() == exception);

This should be `EXCEPTION_ASSERT(!catchScope.exception());` because by the time profiledCall() returns, it should have cleared the exception and return it via the NakedPtr.

> LayoutTests/js/dom/scheduled-action-exception-checks.html:1
> +<script>

Take a look at LayoutTests/js/dom/missing-exception-check-below-queueMicrotask.html and due the same with the stuff surrounding the script: they pass the needed JSC option and also make the test output cleaner.  Please rebase the expected results after you do that.
Comment 3 Michael Saboff 2020-07-24 18:55:47 PDT
Created attachment 405215 [details]
Patch for Landing

(In reply to Mark Lam from comment #2)
> Comment on attachment 405209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405209&action=review
> 
> r=me with fixes.
> 
> > Source/WebCore/bindings/js/ScheduledAction.cpp:122
> > +    EXCEPTION_ASSERT(catchScope.exception() == exception);
> 
> This should be `EXCEPTION_ASSERT(!catchScope.exception());` because by the
> time profiledCall() returns, it should have cleared the exception and return
> it via the NakedPtr.
> 
> > LayoutTests/js/dom/scheduled-action-exception-checks.html:1
> > +<script>
> 
> Take a look at
> LayoutTests/js/dom/missing-exception-check-below-queueMicrotask.html and due
> the same with the stuff surrounding the script: they pass the needed JSC
> option and also make the test output cleaner.  Please rebase the expected
> results after you do that.

Done.
Comment 4 Michael Saboff 2020-07-24 19:09:21 PDT
Committed r264876: <https://trac.webkit.org/changeset/264876>