Closed Bug 902909 Opened 6 years ago Closed 6 years ago

Running ASM.js app crashes Firefox

Categories

(Core :: DOM: IndexedDB, defect)

x86
Windows Vista
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28

People

(Reporter: sys.sgx, Assigned: khuey)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Attachments

(4 files, 2 obsolete files)

Attached image Console of page (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130808030205

Steps to reproduce:

1. Browse to http://www.unrealengine.com/html5/ with version 26a1 trunk build.
2. Wait for resources to load.
3. Check the console (image)
4. When download of data is complete, the browser crashes and has to restart.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
This is on a 2GB RAM machine, Windows x86.
What do you see in about:crashes for that crash?
Looks like an out of memory. How much memory is free before the load of the new website? Can use about:memory at that time to get more detail.
I think that's the case too. The new page loads with about 1.5GB of free RAM.
There are two things that should happen:
1. the asm.js code gets loaded through chunks of data in some way.
2. FF realizes the malloc limits and keeps the browser running.

Point number 2 is more important.
1 is being constantly worked on. Did you try in latest nightly?
2 is hard to do in general, but will improve hugely with e10s which hopefully will happen later this year.
Yes, I'm using the newest version of trunk builds.
Nice to see about the e10s project.
Version: 26 Branch → Trunk
We could also suggest other people to try it on their machines and collect feedback in a structured way. This will measure the overall progress.
This particular crash has:

"OOM Allocation Size": 154556280

So we tried to allocate a 150MB buffer or so...  The problem there is fragmentation: we had 800MB free virtual memory at the time, and nearly a gig of physical memory.

The failing allocation is in AddHelper::DoDatabaseWork in the IDBObjectStore.  I suspect it's this:

  nsAutoArrayPtr<char> compressed(new char[compressedLength]);

This should presumably be using fallible malloc, since compressedLength is, I suspect, under the control of the website.  I'm a little surprised that new[] is using the infallible allocator.  :(

ccing some folks who might know about this code.
Flags: needinfo?(khuey)
Flags: needinfo?(jonas)
Flags: needinfo?(bent.mozilla)
Component: JavaScript Engine → DOM: IndexedDB
Crash due to huge infallible allocation from indexeddb code.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Clearly we should make the allocation fallible, but that's not going to make this app any more functional.  Note that we're OOMing attempting to allocate a buffer to hold the compressed data.  Snappy appears to expose an in-place compression function.  Perhaps we should use that.
Flags: needinfo?(khuey)
Oh, I'm not worried about the app working.  I just don't think it should crash the browser.  ;)
Keywords: crash
Summary: Running ASM.js app crushes Firefox → Running ASM.js app crashes Firefox
Boris, if you're on vacation then who's doing the replying? have you setup an autobot? :)) (ehm...)

That's right, a case like this should not crash the browser. If e10s makes it run on a thread then this will probably be fixed, main thread will always be running, but either way there should be a way to monitor this.
Assignee: nobody → khuey
Agreed that new[] being infallible is surprising... This should probably be audited.
Flags: needinfo?(bent.mozilla)
Attached patch Patch (obsolete) — Splinter Review
Untested, but it should get the job done.
Attachment #788393 - Flags: review?(jonas)
Allocate hard or go home!
Flags: needinfo?(jonas)
Attachment #787504 - Attachment is obsolete: true
Attachment #788393 - Attachment is obsolete: true
Attachment #788964 - Flags: review+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> Backed out for OSX (at least) bustage.

And every other platform...
So the problem here is that fallible_t is not defined, because mozalloc.h (and hence fallible.h) are not getting included here.

How does this code end up using infallible allocators if we're not including the right headers?
Flags: needinfo?(mh+mozilla)
Are you sure it's not simply a namespace issue? dom/indexedDB/IDBObjectStore.cpp doesn't do "using namespace mozilla", so fallible_t alone is unknown. Try mozilla::fallible_t.
Flags: needinfo?(mh+mozilla)
Sure but that doesn't apply to OpenDatabaseHelper.cpp.
IDBObjectStore.cpp is where the build error happened, in the log.
So in the end this is both a namespace issue and an API misuse. new (fallible_t()) doesn't work, while new(fallible_t_instance) does. It is however weird that we don't have a global, shared, such instance. Apparently, all places using fallible allocations define their own static const fallible_t fallible = fallible_t(); (which, btw, is already too much, static const fallible_t fallible; should work equally well)
Do we have any updates?
Thanks
Duplicate of this bug: 922371
Jan can you run with this ... I don't want to fiddle with try to get it to compile on gdb/clang.
Flags: needinfo?(Jan.Varga)
It seems this pattern is used elsewhere a lot.
Flags: needinfo?(Jan.Varga)
So who is driving this now?
Comment on attachment 832190 [details] [diff] [review]
changes required for building on mac

khuey says r+

I'll land the patches today
Attachment #832190 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cdc82950ae45
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Keywords: verifyme
Do we know from which version will this patch take effect?
I just run the app on FF 27 and it produces the same thing.
Status: RESOLVED → REOPENED
Flags: needinfo?(khuey)
Resolution: FIXED → ---
Adding some more info:
The console displays an "error" or compiling the app code (?), then the browser restarts. We should indicate with warning messages about the IndexedDB workings.
(In reply to sys.sgx from comment #38)
> Do we know from which version will this patch take effect?

Yes we know.

The "Target Milestone" field here is set to "mozilla28", so this was included for Firefox 28 first, and there are no status-* flags set under "tracking flags", so it has not been ported to any older versions than that - judging from all that, this is not fixed in 27 but is fixed in 28.

You can check by trying current beta, which already is 28 - and we'd encourage you to do so and report back if that works, as it would be helpful to know if the fix works.
Flags: needinfo?(khuey)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
OK. I've checked the page with FF 30a1, which I guess incorporates the mentioned patch.
The browser no longer crashes, but the app still does not run as expected.

See the new attached file for the console messages.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image FF 30a1 console
Flags: needinfo?(khuey)
UPDATE: the same thing happens when the data are loaded from cache. See attachment 2 [details] [diff] [review] image.
Attached image FF 30a1 cache console
It's not expected to work ... you're still out of memory.  This bug is just about the crash.  Please file a new bug.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(khuey)
Resolution: --- → FIXED
Will do, thanks for the work here. :)
Actually, there's already a bug filed, bug 933398.
Couldn't reproduce the crash on two Vista x86 machines, only "out of memory" error is presented.

Keeping the verifyme keyword so that it could be verified later on this cycle using crashstats.
No crashes present in Socorro for [ @mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | `anonymous namespace''::AddHelper::DoDatabaseWork(mozIStorageConnection*)] signature in the last month: http://goo.gl/WPUxA3 for Firefox 28 builds. Marking as verified based on this result.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.