Bug 186277 - JSLock should clear last exception when releasing the lock
Summary: JSLock should clear last exception when releasing the lock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
: 186221 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-06-04 12:42 PDT by Keith Miller
Modified: 2018-06-11 18:58 PDT (History)
6 users (show)

See Also:

Attachments
Patch (1.36 KB, patch)
2018-06-04 12:44 PDT, Keith Miller 		no flags Details | Formatted Diff | Diff
Patch (1.40 KB, patch)
2018-06-04 14:37 PDT, Keith Miller 		mark.lam: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2018-06-04 12:42:50 PDT 
JSLock should clear last exception when releasing the lock
Comment 1 Keith Miller 2018-06-04 12:44:18 PDT 
Created attachment 341909 [details]
Patch
Comment 2 Mark Lam 2018-06-04 13:25:50 PDT 
Comment on attachment 341909 [details]
Patch

r=me
Comment 3 Mark Lam 2018-06-04 13:27:01 PDT 
Comment on attachment 341909 [details]
Patch

Wait a minute.  What happens when we have a ReleaseAllLocks situation and want to GrabAllLocks again later?
Comment 4 Mark Lam 2018-06-04 13:29:29 PDT 
(In reply to Mark Lam from comment #3)
> Comment on attachment 341909 [details]
> Patch
> 
> Wait a minute.  What happens when we have a ReleaseAllLocks situation and
> want to GrabAllLocks again later?

How about clearing lastException on unlocking JSLock only when vm.topCallFrame is null?
Comment 5 Keith Miller 2018-06-04 14:16:07 PDT 
(In reply to Mark Lam from comment #3)
> Comment on attachment 341909 [details]
> Patch
> 
> Wait a minute.  What happens when we have a ReleaseAllLocks situation and
> want to GrabAllLocks again later?

DropAllLocks doesn't call willReleaseLock()... So it will have the some behavior it has now. willReleaseLock is only called from JSLock::unlock and only if the lock count is going to go to zero.
Comment 6 Keith Miller 2018-06-04 14:37:42 PDT 
Created attachment 341919 [details]
Patch
Comment 7 Keith Miller 2018-06-04 14:39:02 PDT 
Nvm, you're right, we should only drop the last exception if we don't have a top call frame.
Comment 8 Keith Miller 2018-06-04 14:45:54 PDT 
*** Bug 186221 has been marked as a duplicate of this bug. ***
Comment 9 Mark Lam 2018-06-04 14:46:40 PDT 
Comment on attachment 341919 [details]
Patch

r=me
Comment 10 Keith Miller 2018-06-04 14:51:15 PDT 
Committed r232482: <https://trac.webkit.org/changeset/232482>
Comment 11 Radar WebKit Bug Importer 2018-06-04 14:52:21 PDT 
<rdar://problem/40782080>
Comment 12 Saam Barati 2018-06-11 18:58:38 PDT 
Comment on attachment 341919 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSLock.cpp:203
> +        if (!vm->topCallFrame)
> +            vm->clearLastException();

Don't we really just want this in ~VMEntryScope?
Don't we drop locks and regrab them in interesting places. I feel like you're guarding against this with "!vm->topCallFrame", but that's exactly what ~VMEntryScope gives you.