crash in js::jit::JitProfilingFrameIterator::JitProfilingFrameIterator(JSRuntime*, JS::ProfilingFrameIterator::RegisterState const&)

VERIFIED FIXED in mozilla38

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: BenWa, Assigned: djvj)

Tracking

({crash, regression})

Trunk
mozilla38
All
Mac OS X
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-51795c9f-501e-49cd-a450-fc6a32150121.
=============================================================

null pointer crash while browsing randomly.
Flags: needinfo?(kvijayan)
(Assignee)

Updated

3 years ago
Assignee: nobody → kvijayan
Flags: needinfo?(kvijayan)
This is happening very reproducibly for me when loading http://www.thairoom.ca/locations-directions/
(Assignee)

Comment 2

3 years ago
Created attachment 8554586 [details] [diff] [review]
fix-bug-1124070.patch
Attachment #8554586 - Flags: review?(jdemooij)
(Assignee)

Comment 3

3 years ago
Comment on attachment 8554586 [details] [diff] [review]
fix-bug-1124070.patch

Review of attachment 8554586 [details] [diff] [review]:
-----------------------------------------------------------------

Forgot to post description of fix: Issue is handling of baseline frames that are executing eval scripts.  The script in the callee-token is not the same as the script being executed, and it confuses the stack walker.
Blocks: 1057082
Keywords: regression
Version: unspecified → Trunk
Can you write a jit-test for this? It'd be good to verify we can trigger those issues in the shell.
Can we get this fix landed ASAP, one way or the other? (worst-case, maybe with the jit-test in a followup, if it's non-trivial to write a test?)

This causes insta-crashes on various Google properties (particularly on some versions of Google Maps, it seems, though I've also hit it on google calendar), which makes it pretty easy to hit. I've run into this bug at two distinct sites in the past couple hours (technically, I hit bug 1125304 based on crash-signature, but it's likely a dupe of this, as noted there).

(Maybe this only affects nightly testers who have the gecko profiler addon installed -- if so, it's causing less pain than I was thinking when I started writing this comment -- but I bet a lot of Mozilla developers do have that addon installed and are being bitten by this and shaking their fists at nightly stability.)
Causes insta-crashes on https://conc.at directly as well (scroll a bit if it isn't instantaneously triggered).
(likely because there's an embedded google map on that page)
(Assignee)

Updated

3 years ago
Whiteboard: [leave open]
(Assignee)

Comment 8

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Can we get this fix landed ASAP, one way or the other? (worst-case, maybe
> with the jit-test in a followup, if it's non-trivial to write a test?)

I've been having some trouble getting a test case written for this, which has been delaying landing.  I talked it over with jan, and I think we have some approaches for getting the crash testable.

In the meantime, I think I can land the patch and work on the test after.

Updated

3 years ago
Attachment #8554586 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8556015 [details] [diff] [review]
test-bug-1124070.patch

Testing case.  Adds 'readSPSProfilingStack' testing builtin to help trigger use of the jit profiling stack.
Attachment #8556015 - Flags: review?(jdemooij)
(Assignee)

Comment 10

3 years ago
Comment on attachment 8556015 [details] [diff] [review]
test-bug-1124070.patch

Review of attachment 8556015 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/profiler/test-baseline-eval-frame-profiling.js
@@ +2,5 @@
> +setJitCompilerOption("baseline.warmup.trigger", 10);
> +
> +var Q = 'z';
> +function main() {
> +    var x = 9;

The 'var x' and 'var Q' are unnecessary cruft from earlier revisions of the test code.  Will remove.
(Assignee)

Comment 11

3 years ago
Created attachment 8556016 [details] [diff] [review]
test-bug-1124070.patch

Fixed minor nits, spelling mistakes, and incomplete doc sentences.
Attachment #8556015 - Attachment is obsolete: true
Attachment #8556015 - Flags: review?(jdemooij)
Attachment #8556016 - Flags: review?(jdemooij)
Comment on attachment 8556016 [details] [diff] [review]
test-bug-1124070.patch

Review of attachment 8556016 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/jit-test/tests/profiler/test-baseline-eval-frame-profiling.js
@@ +7,5 @@
> +}
> +
> +gczeal(2, 10000);
> +enableSPSProfilingWithSlowAssertions();
> +main();

Nit: can you also add a test that does:

function g() {
    assertEq(JSON.stringify(readSPSProfilingStack()), "<stack>");
}
function f() {
    g();
}
f();

Or something similar. Just to make sure we will notice it when readSPSProfilingStack() breaks.
Attachment #8556016 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 14

3 years ago
> Comment on attachment 8556016 [details] [diff] [review]
> test-bug-1124070.patch
> 
> Review of attachment 8556016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!
> 
> ::: js/src/jit-test/tests/profiler/test-baseline-eval-frame-profiling.js
> @@ +7,5 @@
> > +}
> > +
> > +gczeal(2, 10000);
> > +enableSPSProfilingWithSlowAssertions();
> > +main();
> 
> Nit: can you also add a test that does:
> 
> function g() {
>     assertEq(JSON.stringify(readSPSProfilingStack()), "<stack>");
> }
> function f() {
>     g();
> }
> f();
> 
> Or something similar. Just to make sure we will notice it when
> readSPSProfilingStack() breaks.

I have this test written up, but it's a bit finnicky because the different test environments (e.g. combinations of --ion-eager, --baseline-eager, and garbage collection settings) is causing the asserts to fail.  I don't want to delay the main test landing on account of that, so I'm just landing the regression test and changes required, and will work on fixing up the general test case for landing later.
https://hg.mozilla.org/mozilla-central/rev/26bb4ca5d855
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1125304

Updated

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