1037534 - Crash on http://www.textfiles.com/underconstruction/

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Martin Thomson [:mt:]]

This URL (warning, not friendly to eyes) reliably crashes on the latest android release.  It contains a large number of images, many of them animated gifs.  

Note that most of the images load slowly, and none contain width or height attributes.  That means almost all of them are in the viewport on initial render.  That might indicate resource exhaustion.

Tested on a Galaxy S4.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Not a security bug.

Comment 2

[Aaron Train [:aaronmt]]

I get a flood of 

W/Adreno-EGL(10407): <qeglDrvAPI_eglSwapBuffers:3692>: EGL_BAD_SURFACE
E/BufferQueue(  261): [SurfaceView] dequeueBuffer: can't dequeue multiple buffers without setting the buffer count

E/Surface (10407): dequeueBuffer: IGraphicBufferProducer::requestBuffer failed: 1717840517
E/Surface (10407): dequeueBuffer failed (Unknown error -1717840517)
E/Surface (10407): Surface::unlockAndPost failed, no locked buffer
E/ViewRootImpl(10407): Could not unlock surface
E/ViewRootImpl(10407): java.lang.IllegalArgumentException
E/ViewRootImpl(10407): 	at android.view.Surface.nativeUnlockCanvasAndPost(Native Method)

and then an eventual crash

I/ActivityManager(  793): Process org.mozilla.fennec (pid 10407) (adj 0) has died.
I/WindowState(  793): WIN DEATH: Window{42e7e318 u0 org.mozilla.fennec/org.mozilla.fennec.App}

with a signature

bp-5205c6cb-ccc9-4c9e-a663-e01cf2140711

Comment 3

[George Wright (:gw280) (needinfo me!)]

I can reproduce this locally. Will investigate.

Comment 4

[George Wright (:gw280) (needinfo me!)]

E/IMGSRV  ( 7082): :0: PrepareToDraw: Invalid drawable
E/IMGSRV  ( 7082): :0: glDrawArrays: Can't prepare to draw
E/IMGSRV  ( 7082): :0: WSEGL_GetDrawableParameters: Native buffer became NULL
E/IMGSRV  ( 7082): :0: KEGLGetDrawableParameters: Native window is invalid

Those four lines loop over and over again until Firefox crashes.

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1037534-Ensure-SurfacePipeFactory-MakePipe-handl.patch

I don't think this is sufficient, but it's required.

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to George Wright (:gw280) (:gwright) from comment #5)
> Created attachment 8759818 [details] [diff] [review]
> 0001-Bug-1037534-Ensure-SurfacePipeFactory-MakePipe-handl.patch
> 
> I don't think this is sufficient, but it's required.

Uhhh, what? MakeUnique just calls new, and I don't see where fallible allocation is happening in that code path. Can you explain to me what I'm missing?

Comment 7

[George Wright (:gw280) (needinfo me!)]

So I've been mulling this over; Seth, you're right in that operator new is infallible. I put this check in and it appeared to stop my build/test from crashing, but I'm going to assume because it's OOM related it was random chance that it didn't crash.

I don't really see many good things we can do here. If we're crashing due to OOM because of this, changing the image to be allocated using a fallible allocator will only delay the inevitable as presumably we have no memory so we're screwed anyway. We could maybe switch it to a fallible allocator then raise a memory pressure event?

Alternatively, we can try and come up with a heuristic to determine how many images to load into memory. I don't know what this would look like in practice. I'd suspect something like a hard limit on the number of images and also keep track of how many bytes we've allocated for images and cap that based on system memory.

I don't really see any "good" solutions here. Maybe the best thing to do is just do nothing at all.

Comment 8

[George Wright (:gw280) (needinfo me!)]

So looking at this from another angle, it appears to only crash using a locally-built debug build. Using a nightly build (or a local release build), it gets into a state where the browser UI is still responsive, but the browser is unusable (pages never load etc), and with the same looping messages from comment 4. I haven't been able to reproduce my initial assessment from comment 4 where it would crash afterwards though.

Comment 9

[George Wright (:gw280) (needinfo me!)]

Spoke to Seth about this. Here's the gist of the issue:

01:39 < seth> gw280: we're hitting it on a page with a large number of animated GIFs
01:39 < seth> once we allocate memory for an animated GIF's frames, it stays locked as long as you remain on that page
01:39 < seth> gw280: this is probably the problem. it's *not* that we fail to allocate memory in imagelib. it's that the memory consumed by the GIFs is causing infallible allocation failure elsewhere
01:40 < seth> i'm literally working *right now* towards eliminating that problem with GIFs but it will take a while

Seth is working on this in bug 1257388

Comment 10

[Kuba Niewiarowski]

Tested on Windows and Android, seems to be fixed?

Comment 11

[Martin Thomson [:mt:]]

Something changed. Works for me too.