Bug 157864 - Code that null checks the VM pointer before any use should ref the VM.
Summary: Code that null checks the VM pointer before any use should ref the VM.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-18 15:18 PDT by Mark Lam
Modified: 2016-05-19 14:04 PDT (History)
8 users (show)

See Also:

Attachments
proposed patch. (3.65 KB, patch)
2016-05-18 15:45 PDT, Mark Lam 		keith_miller: review+
Details | Formatted Diff | Diff
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 Mark Lam 2016-05-18 15:18:15 PDT 
Specifically, in JSLock::willReleaseLock() and HeapTimer::timerDidFire().  Otherwise, there's no guarantee that the VM won't be deleted after the null check.

Patch coming.
Comment 1 Mark Lam 2016-05-18 15:21:28 PDT 
<rdar://problem/26129156>
Comment 2 Mark Lam 2016-05-18 15:45:45 PDT 
Created attachment 279304 [details]
proposed patch.

Still need to run tests.
Comment 3 WebKit Commit Bot 2016-05-18 15:48:00 PDT 
Attachment 279304 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSLock.cpp:180:  'vm' is incorrectly named. It should be named 'protector' or 'protectedVm'.  [readability/naming/protected] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2016-05-18 15:49:04 PDT 
Comment on attachment 279304 [details]
proposed patch.

Nice!
Comment 5 Keith Miller 2016-05-18 15:54:31 PDT 
Comment on attachment 279304 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:3
> +        Code that need to null check the VM pointer before use should ref the VM.

I think this would be less confusing as "Code that null checks the VM pointer before any use should ref the VM."
Comment 6 Keith Miller 2016-05-18 15:55:00 PDT 
Comment on attachment 279304 [details]
proposed patch.

r=me too.
Comment 7 Mark Lam 2016-05-18 16:27:10 PDT 
(In reply to comment #5)
> Comment on attachment 279304 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279304&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        Code that need to null check the VM pointer before use should ref the VM.
> 
> I think this would be less confusing as "Code that null checks the VM
> pointer before any use should ref the VM."

I'll make the change.
Comment 8 Mark Lam 2016-05-19 14:03:35 PDT 
The patch has passed the layout tests and JSC tests on x86_64.  I also did an ad hoc smoke test by running a few apps with it on ARM64.
Comment 9 Mark Lam 2016-05-19 14:04:16 PDT 
Landed in r201180: <http://trac.webkit.org/r201180>.