Closed Bug 1443555 Opened 2 years ago Closed 2 years ago

Assertion failure: !hadEvaluationError(), after error during module evaluation (see comment 6)

Categories

(Core :: JavaScript Engine, defect, P2)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mrspeaker, Assigned: jonco)

Details

Crash Data

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180208173149

Steps to reproduce:

There seems to be an issue (on Linux at least) that is a combination of Native Module loading, `const` reassigning and requestAnimationFrame. 

I am loading a page with `<script type="module">` that looks like this:

```js
import A from "./A.js";
const a = A;
requestAnimationFrame(loopy);
a = 2;
function loopy() {
  requestAnimationFrame(loopy);
  A;
}
```
The file A.js is one line: `export default 1;`.


Actual results:

If the requestAnimationFrame is scheduled before trying to reassign the const, then when I make any reference to A inside the loop - the browser tab will crash. (I sent a report with breakpad id: bp-ed23719c-4f33-4812-93ad-5d29d0180306)


Expected results:

It should throw a "TypeError: invalid assignment to const `a'" and not crash.
I cannot reproduce the crash on Ubuntu 16.04 x64 with Firefox Release 58.0.2 using the STR from description.

I will assign a component based on reporter's crash signature.
Crash Signature: [@ js::ModuleEnvironmentObject::importBindings]
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Hi mrspeaker! Thanks for the great bug report. From the crash dumps, this is clearly a real, specific issue--although we may 
already have fixed it. Looking now.
Assignee: nobody → jorendorff
Component: JavaScript Engine: JIT → JavaScript Engine
Priority: -- → P2
During baseline compilation of the function,

    js::jit::BaselineCompiler::emit_JSOP_GETIMPORT
    -> js::ModuleEnvironmentObject::lookupImport
    -> js::ModuleEnvironmentObject::importBindings

From there, due to inlining, I can't tell, but I think

    either  https://searchfox.org/mozilla-central/source/js/src/vm/EnvironmentObject.cpp#470
    or      https://searchfox.org/mozilla-central/source/js/src/builtin/ModuleObject.cpp#840

One of those calls to js::NativeObject::getReservedSlot() is receiving this=null, leading to a crash reading this->shape here:

    js::shadow::Object::numFixedSlots()
    https://searchfox.org/mozilla-central/source/js/src/jsfriendapi.h#611

at address 0x8 as shown in the dump.

So either GetModuleEnvironmentForScript() is returning null, or the ModuleEnvironmentObject's MODULE_SLOT slot is null. Neither one seems exactly likely, though...
André, would you mind taking a look?
Flags: needinfo?(andrebargull)
mrspeaker: In the meantime, if you happen to have free time to help track this down, there are a few things I'm curious about:

*   Does it happen in a fresh profile? You can use this command to make one: firefox -no-remote -P

*   Does it happen in Firefox Nightly?

*   Are you looking at these pages using a file: URL, or a local web server? What kind?

Whatever info you can conveniently get is fine, don't feel like you have to get all the answers :)
I can't reproduce in Firefox59, bug 1420420 probably fixed the issue. 

We should still leave this bug open, because the test case triggers this assertion [1] in debug builds. AFAICT that's a bogus assertion and we can safely remove it.

[1] https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/js/src/builtin/ModuleObject.cpp#820



Shell test case, run with `js -m main.js`:

main.js
---
import A from "./A.js";

const a = A;

function requestAnimationFrame(f) { Promise.resolve().then(f); };

requestAnimationFrame(loopy);
a = 2;
function loopy() {
    requestAnimationFrame(loopy);
    A;
}
---

A.js:
---
export default 1;
---
Flags: needinfo?(andrebargull)
mrspeaker: Please see comment 5 and comment 6. Thanks again for the bug report.
Flags: needinfo?(mrspeaker)
cc-ed :jonco because it also involves modules.
Sorry about the long delay in replying - unfortunately it was unavoidable.

Some point in the last month I had upgraded to version 59 and I can no longer replicate this crash. But if it is important, previously I COULD recreate this with a new profile, and it was while running the file locally (not served) from `file://` (I didn't test it over a file server).

I'm not sure what the rules for bugs are here - but I'll resolve this as INVALID now. Thanks for your efforts on this!
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(mrspeaker)
Resolution: --- → INVALID
We still have an overeager assertion that this testcase fails, so I'm reopening to fix that.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
Summary: tab crash with requestAnimationFrame after const reassignment → Assertion failure: !hadEvaluationError(), after error during module evaluation (see comment 6)
Flags: needinfo?(jcoppeard)
I agree that the assertion is bogus.
Assignee: jorendorff → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8970927 - Flags: review?(andrebargull)
Comment on attachment 8970927 [details] [diff] [review]
bug1443555-module-assert

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

Thanks!

::: js/src/builtin/ModuleObject.cpp
@@ +812,5 @@
>  ModuleEnvironmentObject*
>  ModuleObject::environment() const
>  {
> +    // Note that this it's valid to call this even if there was an error
> +    // evaluating the module.

Nit: The first "this" should be removed.

::: js/src/jit-test/tests/modules/bug-1443555.js
@@ +1,5 @@
> +// |jit-test| error: TypeError
> +
> +"use strict";
> +
> +setJitCompilerOption("baseline.warmup.trigger", 0);

Why do we need to modify this option? IIRC the assertion was also triggered with the default settings.
Attachment #8970927 - Flags: review?(andrebargull) → review+
(In reply to André Bargull [:anba] from comment #12)
> Why do we need to modify this option? IIRC the assertion was also triggered
> with the default settings.

I removed the call to requestAnimationFrame in loopy which made the test loop for ever.  The problematic call to ModuleObject::environment() comes when we try to baseline compile the module.  These changes made the test fail without looping.
(In reply to Jon Coppeard (:jonco) from comment #13)
> I removed the call to requestAnimationFrame in loopy which made the test
> loop for ever.  The problematic call to ModuleObject::environment() comes
> when we try to baseline compile the module.  These changes made the test
> fail without looping.

Ah, I see. Thanks for the explanation! :-)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf3be36c24f
Remove bogus assertion to allow getting the module environment even if there was an error evaluating the module r=anba
https://hg.mozilla.org/mozilla-central/rev/dbf3be36c24f
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.