Bug 16909 – REGRESSION: Amazon.com crash (ActivationImp)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Cameron Zwarich (cpst)]

Navigate to http://www.amazon.com/ and click on the Amazon logo in the top left corner. Go back and do it again. Repeating this a small number of times leads to a crash.

I haven't tested it yet on an old nightly build, but this crash was probably introduced by the ActivationImp tear-off patch r29425. The address it dies on is at a small offset from 0, it can happen at a number of places in the code, and it does not quite happen deterministically, so it looks like a missed GC mark along the lines of bug 16868 or bug 16871. I'll post a stack trace with a modified GC that collects after every allocation.

[Cameron Zwarich (cpst)]

<rdar://problem/5691998>

[Cameron Zwarich (cpst)]

Created attachment 18509 [details]
Stack trace

Strangely enough, I can't actually get it to crash with the GC modified to always collect after an allocation. Perhaps it is a timing issue that the extra garbage collection eliminates.

Here is a stack trace of a typical crash.

[Maciej Stachowiak]

Alternate steps for a crash on Amazon (which are more reliable for me):

1. Go to Amazon.com
2. Search for "Hoboken Chicken Emergency"
3. Open the link for the first product result in a background tab.

[Cameron Zwarich (cpst)]

Maciej's instructions reliably reproduce the crash for me, with a similar looking stack trace to what I posted. Still, changing it to garbage collect every allocation makes the crash impossible to reproduce.  Instead of crashing, it prints a JavaScript object such as

function n2RunEvent(sWhen) { goN2Initializer.run(sWhen); }

or

[object Object]

at the top of the page.

[Cameron Zwarich (cpst)]

Created attachment 18518 [details]
ExecState activity log

Tearing off every ActivationImp as soon as it is created fixes the crash, which suggests that the problem is due to a missed reference to an ActivationImp from an ExecState that should be explicitly mark()'d but isn't. Tearing off the activation puts a pointer into the GC heap in m_activation, which is caught by the conservative collector.

Maciej suggested I log ExecState creations, deletions, and markings so that we can check for anything suspicious. I've attached such a log from a session that crashed.

[Maciej Stachowiak]

Created attachment 18520 [details]
execstate activity log annotated with GC START and GC END markers

[Maciej Stachowiak]

Created attachment 18521 [details]
perl script to find inconsistencies

[Maciej Stachowiak]

Created attachment 18522 [details]
results of perl script (look for the XXX)

[Maciej Stachowiak]

With more investigation, I found that some live ExecStates fail to get marked specifically because script execution (via the <script> element) sometimes breaks both the callingExec and savedExec chains. I have a fix but it might be bad for performance - will have to test.

[Maciej Stachowiak]

Created attachment 18523 [details]
this patch is a .4% SunSpider regression so it can't go in as-is but I think I know how to make it viable

[David Kilzer (:ddkilzer)]

<rdar://problem/5696387>

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> <rdar://problem/5691998>

Cameron, please add the "InRadar" keyword to a bug if you file them in Radar.  Thanks!

[Geoffrey Garen]

Created attachment 18549 [details]
.7% speedup - do we have a winner?

[Geoffrey Garen]

Created attachment 18552 [details]
The accompanying WebCore patch

[Geoffrey Garen]

Created attachment 18556 [details]
This patch builds and passes regression tests

[Maciej Stachowiak]

Comment on attachment 18556 [details]
This patch builds and passes regression tests

r=me

Thanks for finishing this off. I'd guess we can also reinstate the LocalStorage optimization since all ExecStates will be tracked?

[Geoffrey Garen]

I'm working on landing this now.

With a little work, yes, I believe we can re-instate the LocalStorage buffer caching optimization.

[Geoffrey Garen]

Committed revision 29710.