Bug 186277

Summary: JSLock should clear last exception when releasing the lock
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, mark.lam, msaboff, saam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch mark.lam: review+

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.