If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Profiler crashes in Box2D demo

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
P2
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: azakai, Assigned: billm)

Tracking

({crash})

unspecified
mozilla21
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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
Assignee: nobody → anton
Priority: -- → P2

Comment 1

5 years ago
On Windows: bp-82fed63c-7318-4864-8ede-66d262130121.
Severity: normal → critical
Crash Signature: [@ JSContext::malloc_(unsigned int)] [@ js::Vector<wchar_t, int, js::ContextAllocPolicy>::convertToHeapStorage(unsigned int)]
Keywords: crash, reproducible
OS: Linux → All
Hardware: x86 → All
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

5 years ago
Which Firefox versions does this affect?
(Reporter)

Comment 4

5 years ago
I saw the crash on nightly. I think the profiler is present only there?
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.

Updated

5 years ago
Keywords: reproducible
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.
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.
Component: Developer Tools: Profiler → JavaScript Engine
Product: Firefox → Core
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
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

5 years ago
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.
Assignee: anton → general

Comment 11

5 years ago
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
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
(Assignee)

Comment 13

5 years ago
Created attachment 714184 [details] [diff] [review]
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.)
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #714184 - Flags: review?(dvander)
Attachment #714184 - Flags: review?(dvander) → review+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7ef6d06439
(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.
(Assignee)

Comment 16

5 years ago
(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.
(Reporter)

Comment 17

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/ce7ef6d06439
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

4 years ago
Duplicate of this bug: 839581
You need to log in before you can comment on or make changes to this bug.