832812 - Profiler crashes in Box2D demo

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Alon Zakai (:azakai)]

STR:

1. Load http://syntensity.com/static/box2d.html
2. Open console and click to start the profiler.

It consistently crashes,

https://crash-stats.mozilla.com/report/index/bp-e1e9e841-5ffb-44be-b0e4-1f1a62130121
https://crash-stats.mozilla.com/report/index/1775eb39-fbed-4a9b-b1d3-a2b2d2130121

Comment 1

[Scoobidiver (away)]

On Windows: bp-82fed63c-7318-4864-8ede-66d262130121.

Comment 2

[Anton Kovalyov (:anton)]

I checked it with Gecko Profiler addon and couldn't reproduce the crash. So it seems to be specific either to some specific flags we use or, less likely, our profiler UI. Will be posting updates as I go.

Comment 3

[Robert Kaiser]

Which Firefox versions does this affect?

Comment 4

[Alon Zakai (:azakai)]

I saw the crash on nightly. I think the profiler is present only there?

Comment 5

[Anton Kovalyov (:anton)]

Yes, current Nightly. Also, a couple of meetings ago we decided to pref the Profiler off for the upcoming release.

Curiously enough, I was playing with this bug yesterday and after some time it stopped being reproducible. Not sure what caused it.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I just hit this crash, didn't have the devtools profiler running, but I do have the profiler add-on installed and it now does periodic updates to the server as I understand it.

Comment 7

[Benoit Girard (:BenWa)]

As far as the JS engine is concerned the profiler add-on or the devtools profiler are identical. They both use a single toggle option.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

My crash report lead me to this bug. Yesterday Firefox would crash whenever I went to the mozilla.dev.platform Google group. On today's Nightly (just updated) it hasn't happened yet so far though.

Example report: https://crash-stats.mozilla.com/report/index/8e3f697c-077a-4093-b9ec-384c12130205

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Nevermind that bit about today's Nightly, I just got it again:
https://crash-stats.mozilla.com/report/index/bp-b6721802-1df9-4c52-91ca-ce5fb2130206

I've turned the profiler addon off for now. So far so good.

Comment 10

[Mayank Bansal]

1. Start fresh profile.
2. Install SPS profiler. Enable it.
3. Go to google groups.
4. Switch between "standard view" and "compact list view"  2-3 times.
5. Crash.

Comment 11

[Mayank Bansal]

also occuring on the new Google's  "enter web lab" demo.  Enable the profiler on the initial load page. Nightly crashes.

https://crash-stats.mozilla.com/report/index/bp-56eba11f-6145-4ea0-a517-a72762130213

Comment 12

[Rob Campbell [:rc] (:robcee)]

thanks for the additional failures. Stacks appear to be crashing in convertToHeapStorage around line 643.

http://mxr.mozilla.org/mozilla-central/source/js/public/Vector.h#643

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: workaround

This made me crash every time in google groups. The problem is that we're using a hashtable in the profiler data structure (strings) from within off-thread compilation. There hashtable is shared with the main thread, and there's no lock protecting it.

This patch disables off-thread compilation when profiling is enabled. The right way to do this would be to refactor the code to use a separate string table from the compilation thread. However, I don't know this code well and I'd rather just fix it immediately. (There's also a problem here if someone enables the profiler while we're doing off-thread compilation. Let's just pretend that doesn't happen.)

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7ef6d06439

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Bill McCloskey (:billm) from comment #13)
> This patch disables off-thread compilation when profiling is enabled.

What's the performance impact of that? There's already some overhead so keeping the profiling cost as low as possible lets us use the profiler as an 'always-on look for bad stuff' tools.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Benoit Girard (:BenWa) from comment #15)
> (In reply to Bill McCloskey (:billm) from comment #13)
> > This patch disables off-thread compilation when profiling is enabled.
> 
> What's the performance impact of that? There's already some overhead so
> keeping the profiling cost as low as possible lets us use the profiler as an
> 'always-on look for bad stuff' tools.

I think there's a modest performance cost. Maybe a few percent on SunSpider, and bigger losses for games that have tons of JS code.

I filed bug 841646 to fix it the right way.

Comment 17

[Alon Zakai (:azakai)]

(In reply to Bill McCloskey (:billm) from comment #16)
> (In reply to Benoit Girard (:BenWa) from comment #15)
> > (In reply to Bill McCloskey (:billm) from comment #13)
> > > This patch disables off-thread compilation when profiling is enabled.
> > 
> > What's the performance impact of that? There's already some overhead so
> > keeping the profiling cost as low as possible lets us use the profiler as an
> > 'always-on look for bad stuff' tools.
> 
> I think there's a modest performance cost. Maybe a few percent on SunSpider,
> and bigger losses for games that have tons of JS code.
> 

A large potential issue is that the maximum script size for compilation is higher with off-thread compilation. Can we perhaps disable off-thread compilation when profiling but keep using the higher script size limit ? Compilation pauses during profiling are not terrible, and this way we would be profiling something closer to how the code normally runs.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce7ef6d06439