Bug 161760

Summary: Heap::isMarked() shouldn't pay the price of concurrent lazy flipping
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, keith_miller, mark.lam, msaboff, ossy, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=161794
Attachments:
Description Flags
the patch
none
the patch
mark.lam: review+, buildbot: commit-queue-
Archive of layout-test-results from ews115 for mac-yosemite
none
better patch none

Description Filip Pizlo 2016-09-08 14:07:19 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-09-08 14:36:12 PDT
Created attachment 288323 [details]
the patch
Comment 2 Filip Pizlo 2016-09-08 14:42:40 PDT
Created attachment 288326 [details]
the patch
Comment 3 Mark Lam 2016-09-08 14:45:16 PDT
Comment on attachment 288326 [details]
the patch

r=me
Comment 4 Build Bot 2016-09-08 16:06:46 PDT
Comment on attachment 288326 [details]
the patch

Attachment 288326 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2036936

New failing tests:
inspector/heap/tracking.html
Comment 5 Build Bot 2016-09-08 16:06:51 PDT
Created attachment 288345 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Filip Pizlo 2016-09-08 16:27:02 PDT
Created attachment 288348 [details]
better patch

The bots found one more isMarked.
Comment 7 Filip Pizlo 2016-09-08 18:24:33 PDT
Landed in https://trac.webkit.org/changeset/205683
Comment 9 Mark Lam 2016-09-09 06:02:06 PDT
The JSC test failures were due to an uninitialized isGCThread.  Landed r205740: <http://trac.webkit.org/r205740> to add the null check.  Note that there's already precedence for this type of null check in registerGCThread().

That said, I'm not sure that this is the right fix.  The real issue is that initializeGCThreads() was never called by the jsc shell.  With Fil's patch, is it ok to still allow isGCThread to be uninitialized?  I suspect not, but I'll let Fil look into it further.
Comment 10 Filip Pizlo 2016-09-09 08:44:23 PDT
(In reply to comment #9)
> The JSC test failures were due to an uninitialized isGCThread.  Landed
> r205740: <http://trac.webkit.org/r205740> to add the null check.  Note that
> there's already precedence for this type of null check in registerGCThread().
> 
> That said, I'm not sure that this is the right fix.  The real issue is that
> initializeGCThreads() was never called by the jsc shell.  With Fil's patch,
> is it ok to still allow isGCThread to be uninitialized?  I suspect not, but
> I'll let Fil look into it further.

Interesting.  Looks like the problem is that we're only calling initializeMainThread() conditionally in jsc.
Comment 11 Filip Pizlo 2016-09-09 08:49:55 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > The JSC test failures were due to an uninitialized isGCThread.  Landed
> > r205740: <http://trac.webkit.org/r205740> to add the null check.  Note that
> > there's already precedence for this type of null check in registerGCThread().
> > 
> > That said, I'm not sure that this is the right fix.  The real issue is that
> > initializeGCThreads() was never called by the jsc shell.  With Fil's patch,
> > is it ok to still allow isGCThread to be uninitialized?  I suspect not, but
> > I'll let Fil look into it further.
> 
> Interesting.  Looks like the problem is that we're only calling
> initializeMainThread() conditionally in jsc.

Aha!  Actually, I think that the real issue is that because the jsc shell never called initializeGCThreads(), my testing on the shell never caught the crash that the bots caught.  That's because if initializeGCThreads() is never called, then isGCThread is null, so we never get to crash on **isGCThread because we'll fall out before we get there.

The right fix is to do both what you did, and to call initializeMainThread() (or initializeGCThreads()) in jsc.cpp.
Comment 12 Mark Lam 2016-09-09 10:20:47 PDT
(In reply to comment #11)
> Aha!  Actually, I think that the real issue is that because the jsc shell
> never called initializeGCThreads(), my testing on the shell never caught the
> crash that the bots caught.  That's because if initializeGCThreads() is
> never called, then isGCThread is null, so we never get to crash on
> **isGCThread because we'll fall out before we get there.
> 
> The right fix is to do both what you did, and to call initializeMainThread()
> (or initializeGCThreads()) in jsc.cpp.

Fil, based on the new GC changes, is it a requirement that isGCThread be properly initialized (as the GC depends on it for correctness)?  The reason I ask is because I think the GTK port does not initialize isGCThread.  If it is now a requirement, I would like to give them a heads up.