Closed Bug 1372258 Opened 7 years ago Closed 6 years ago

Page with script type="module" crashes tab/browser

Categories

(Core :: JavaScript Engine, defect, P2)

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: sergiomdgomes, Assigned: jonco)

References

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.24 Safari/537.36

Steps to reproduce:

1. Set dom.moduleScript.enabled to true in about:config
2. Load https://importtests-d1bd8.firebaseapp.com/


Actual results:

Tab or browser crashes. Happens in Firefox 54.0, FirefoxDeveloperEdition 54.0b14, and FirefoxNightly 55.0a1 (2017-06-12).


Expected results:

Page loads and outputs string of the type "Today is Monday." to console.

This page is meant as a stress test for native ECMAScript module loading. It loads a modified version of the moment.js library and attempts to print the current day to the console.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Note: Please ignore user agent in bug report; bug filed with different browser.
Here is another test case, in case it helps: https://importteststhree.firebaseapp.com

This one imports a modified three.js. Same crashing behaviour.
Can confirm that the https://importteststhree.firebaseapp.com testcase crashes the tab in Nightly 2017-07-21 with dom.moduleScripts.enabled=true.

No crash with dom.moduleScripts.enabled=false.
Still crashing with 57.0a1 (2017-08-16).
Confirm crash. Since it is behind a pref setting as P2.
Assignee: nobody → jcoppeard
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Blocks: modules-0
Doesn't crash for me.
(In reply to Tom Schuster [:evilpie] from comment #6)
> Doesn't crash for me.

https://crash-stats.mozilla.com/report/index/57a37f8b-4580-47f5-a9d4-b38a50171204 (normal build)


When I tested it in an opt-debug build, I was able to pinpoint it to a nullptr access in MemoryCounter::shouldTriggerGC(...) when called from JS::Zone::updateMemoryCounter(...):

(gdb) br JS::Zone::updateMemoryCounter if !this->runtime_
Haltepunkt 1 at 0x7fffe9019ce0: file /home/andre/hg/mozilla-inbound/js/src/gc/Zone.h, line 430.
(gdb) c
Continuing.
...
Thread 1 "firefox" hit Breakpoint 1, JS::Zone::updateMemoryCounter (this=this@entry=0x7fffcc5d3000, counter=..., nbytes=nbytes@entry=2048) at /home/andre/hg/mozilla-inbound/js/src/gc/Zone.h:430
430	    void updateMemoryCounter(js::gc::MemoryCounter& counter, size_t nbytes) {
(gdb) bt
#0  0x00007fffe9019ce0 in JS::Zone::updateMemoryCounter(js::gc::MemoryCounter&, unsigned long) (this=this@entry=0x7fffcc5d3000, counter=..., nbytes=nbytes@entry=2048)
    at /home/andre/hg/mozilla-inbound/js/src/gc/Zone.h:430
#1  0x00007fffe90cfd79 in js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(int, js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) (nbytes=2048, this=<optimised out>) at /home/andre/hg/mozilla-inbound/js/src/gc/Zone.h:465
#2  0x00007fffe90cfd79 in js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(int, js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) (numElems=<optimised out>, this=0x7fffcc5d3000) at /home/andre/hg/mozilla-inbound/js/src/vm/MallocProvider.h:64
#3  0x00007fffe90cfd79 in js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(int, js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) (numElems=<optimised out>, this=0x7fffcc5d3000) at /home/andre/hg/mozilla-inbound/js/src/vm/MallocProvider.h:132
#4  0x00007fffe90cfd79 in js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(int, js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) (numElems=64, this=0x7fffc3ae9280) at /home/andre/hg/mozilla-inbound/js/src/gc/Zone.h:979
#5  0x00007fffe90cfd79 in js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(int, js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) (reportFailure=js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::ReportFailure, capacity=64, alloc=...)
    at /home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:1237
#6  0x00007fffe90cfd79 in js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::changeTableSize(int, js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::FailureBehavior) (this=this@entry=0x7fffc3ae9280, deltaLog2=<optimised out>, reportFailure=reportFailure@entry=js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::ReportFailure)
    at /home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:1499
#7  0x00007fffe90b0885 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (reportFailure=js::detail::HashTable<js::HashMapEntry<jsid, js::IndirectBindingMap::Binding>, js::HashMap<jsid, js::IndirectBindingMap::Binding, js::DefaultHasher<jsid>, js::ZoneAllocPolicy>::MapHashPolicy, js::ZoneAllocPolicy>::ReportFailure, this=0x7fffc3ae9280) at /home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:1545
#8  0x00007fffe90b0885 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (l=..., this=0x7fffc3ae9280)
    at /home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:1854
#9  0x00007fffe90b0885 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (v=<unknown type in /home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so, CU 0x25861e5c, DIE 0x25aacc0d>, k=<synthetischer Zeiger>, this=0x7fffc3ae9280)
    at /home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:253
#10 0x00007fffe90b0885 in js::IndirectBindingMap::putNew(JSContext*, JS::Handle<jsid>, JS::Handle<js::ModuleEnvironmentObject*>, JS::Handle<jsid>) (this=0x7fffc3ae9280, cx=cx@entry=0x7fffdf924000, name=..., name@entry=$jsid("encodings_fragment"), environment=..., environment@entry=(js::ModuleEnvironmentObject * const) 0x7fffcc703dc0 [object ModuleEnvironmentObject] delegate, localName=..., 
    localName@entry=$jsid("default")) at /home/andre/hg/mozilla-inbound/js/src/builtin/ModuleObject.cpp:350
#11 0x00007fffe9636177 in js::ModuleEnvironmentObject::createImportBinding(JSContext*, JS::Handle<JSAtom*>, JS::Handle<js::ModuleObject*>, JS::Handle<JSAtom*>) (this=(js::ModuleEnvironmentObject * const) 0x7fffc5496c40 [object ModuleEnvironmentObject] delegate, cx=0x7fffdf924000, importName=..., importName@entry="encodings_fragment", module=..., 
    module@entry=(js::ModuleObject * const) 0x7fffcc8a7380 [object Module], localName=..., localName@entry="default") at /home/andre/hg/mozilla-inbound/js/src/vm/EnvironmentObject.cpp:496
#12 0x00007fffe972beb0 in intrinsic_CreateImportBinding(JSContext*, unsigned int, JS::Value*) (cx=0x7fffdf924000, argc=<optimised out>, vp=<optimised out>)
    at /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:2056
#13 0x00003d62b145b181 in  ()
#14 0x0000000000000000 in  ()
(In reply to André Bargull [:anba] from comment #7)
I'm seeing crashes like this too, on macosx optdebug builds only.  Investigating.
The problem is that modules parsed on the background thread have their bindings map use the parse zone as their allocator.  After parsing is finished and this zone destroyed any attempt to create import bindings results in a use after free.

The patch lazily creates the bindings map when adding entries, since we always have the final zone by the time this happens.

The alternative would be to somehow the zone used by the hash map when we merge compartments.  We do this in other places but there's no simple way to do that here.
Attachment #8935455 - Flags: review?(andrebargull)
Comment on attachment 8935455 [details] [diff] [review]
bug1372258-lazy-bindings-map

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

r+ if regression test added. For example this one seems to cause the same error. :-)

---
if (helperThreadCount() == 0)
    quit();

// Overwrite built-in parseModule with off-thread module parser.
function parseModule(source) {
    offThreadCompileModule(source);
    return finishOffThreadModule();
}

// Test case derived from: js/src/jit-test/tests/modules/many-imports.js

// Test importing an import many times.

load(libdir + "dummyModuleResolveHook.js");

const count = 1024;

let a = moduleRepo['a'] = parseModule("export let a = 1;");

let s = "";
for (let i = 0; i < count; i++) {
    s += "import { a as i" + i + " } from 'a';\n";
    s += "assertEq(i" + i + ", 1);\n";
}
let b = moduleRepo['b'] = parseModule(s);

b.declarationInstantiation();
b.evaluation();
---

::: js/src/builtin/ModuleObject.cpp
@@ +772,2 @@
>          ReportOutOfMemory(cx);
>          js_delete<IndirectBindingMap>(bindings);

Nit: |js_delete| is no longer needed here, because |bindings| is now always nullptr when we reach this point.
Attachment #8935455 - Flags: review?(andrebargull) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe87a120bd51
Lazily initialise module binding maps so they are not allocated on a background thread r=anba
(In reply to André Bargull [:anba] from comment #10)
Thanks for the testcase.
https://hg.mozilla.org/mozilla-central/rev/fe87a120bd51
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.