Bug 161760 – Heap::isMarked() shouldn't pay the price of concurrent lazy flipping

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 288323 [details]
the patch

[Filip Pizlo]

Created attachment 288326 [details]
the patch

[Mark Lam]

Comment on attachment 288326 [details]
the patch

r=me

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

Created attachment 288348 [details]
better patch

The bots found one more isMarked.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/205683

[Csaba Osztrogonác]

(In reply to comment #7)
> Landed in https://trac.webkit.org/changeset/205683

It made 2 JSC tests fail/timeout/crash/whatever on debug bots:
- https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/3643
- https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/4032
- https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/10586
- https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20JSC%20%28Tests%29/builds/6940

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

(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.

[Mark Lam]

(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.