736481 - Fennec process is stopped when loading a WebGL page due to "Low Memory" (regression from Maple merge)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Firefox 14.0a1 (2012-03-16)
Device: Samsung Nexus S
OS: Android 2.3.6

Steps to reproduce:
1. Open Fennec
2. Browse to http://people.mozilla.org/~mwargers/tests/layout/layout_demos.htm
3. Tap on a WebGL link (for me it works with http://media.tojicode.com/q3bsp/)
4. Rotate the device for a couple of times
5. Wait 1 min or less

Expected result:
Fennec process should not die after step 5.

Actual result:
After step 5, Fennec is closed:

I/ActivityManager(  110): Low Memory: No more background processes.
I/ActivityManager(  110): Process org.mozilla.fennec (pid 3244) has died.
E/InputDispatcher(  110): channel '408ebec8 org.mozilla.fennec/org.mozilla.fennec.App (server)' ~ Consumer closed input channel or an error occurred.  events=0x8
E/InputDispatcher(  110): channel '408ebec8 org.mozilla.fennec/org.mozilla.fennec.App (server)' ~ Channel is unrecoverably broken and will be disposed!
I/WindowManager(  110): WIN DEATH: Window{408ebec8 org.mozilla.fennec/org.mozilla.fennec.App paused=false}
I/WindowManager(  110): WIN DEATH: Window{408f4bb0 SurfaceView paused=false}

Note:
For this device it's always reproducible using the link from step 3. However on other devices will occur using a different one.

Comment 1

[Aaron Train [:aaronmt]]

That demo is highly resource intensive on the desktop as it is a desktop oriented demo; it's no surprise at all that on a mobile device you OOM. This bug is likely invalid.

Comment 2

[Cristian Nicolae (:xti)]

(In reply to Aaron Train [:aaronmt] from comment #1)
> That demo is highly resource intensive on the desktop as it is a desktop
> oriented demo; it's no surprise at all that on a mobile device you OOM. This
> bug is likely invalid.

On other devices will not OOM on that demo page, but will do on other page from the list. I logged this bug for http://goo.gl/8aw9q.

Comment 3

[Martijn Wargers (dead)]

See also bug 723369.

Comment 4

[Martijn Wargers (dead)]

If Fennec can't handle most of these webgl demos because of oom issues, perhaps it would be better to disable webgl completely for Fennec then?

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #4)
> If Fennec can't handle most of these webgl demos because of oom issues,
> perhaps it would be better to disable webgl completely for Fennec then?

This cannot be the way forward. We could potentially add a minimum memory requirement to initialization, but a WebGL context doesn't by itself take up much space. Nexus S has a 800x480 screen and 512MB of RAM. A high estimate for a WebGL context would be 10MB for a screen of this size.

The real question is whether we are using significantly more RAM than we need to be.

Comment 6

[Martijn Wargers (dead)]

This bug has blocking-fennec1.0: -.
That means it is ok-ish to ship Fennec Native release with this bug open.
I'm just wondering what would be better, ship a release where lots of webgl demos seem to crash Fennec or disable webgl for that release entirely.

Of course fixing this bug is the ideal solution.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Is that any specific to WebGL or is it just something that happens when loading any memory-heavy page? 

The WebGL link you gave in comment 0 is particularly memory-heavy. Do you get the same crash if you load a lightweight WebGL page instead, such as

   http://people.mozilla.org/~bjacob/webgl-spotlight-js-2012/webgltrianglecolor.html

?

I'm trying to figure if there's something that makes WebGL contexts inherently memory intensive on Android or if it's just that heavy 3D games are heavy.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Also, is this a recent regression? Could you bisect it?

Comment 9

[Martijn Wargers (dead)]

No, I don't see a crash happening with the lightweight WebGL page from comment 7. I'll try to find out if there is a regression range here.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Also, when it crashes, does this bring up the crash reporter? Do you see crash links in about:crashes?

Comment 11

[Martijn Wargers (dead)]

Firefox 12b XUL doesn't seem to crash.

I tested this with this url: http://webglsamples.googlecode.com/hg/aquarium/aquarium.html
Using 1000 fish and then zooming in.

I'll try to get a more narrow regression range here.

Comment 12

[Martijn Wargers (dead)]

No, current trunk Fennec Native just dies without the Crash Reporter appearing. Nothing shows up in about:crashes.

Comment 13

[Martijn Wargers (dead)]

Fennec 13.0a from 2012-03-27 doesn't have this issue.

Fennec trunk from 2012-0314 03-11-39 seems not to have this issue.
Fennec trunk from 2012-03-14 13-06-26 seems to have this issue.
So it looks like a regression from the Maple merge.

Based on the fact that this is a regression, re-asking for blocking.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #13)
> Fennec trunk from 2012-0314 03-11-39 seems not to have this issue.
> Fennec trunk from 2012-03-14 13-06-26 seems to have this issue.
> So it looks like a regression from the Maple merge.

I know this is asking for much, but could you bisect inside the Maple branch? Hopefully the Maple nightly builds are still available on FTP.

Comment 15

[Joe Drew (not getting mail)]

Maybe on low memory, we can kill the context and send context-lost to the demo?

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Joe Drew (:JOEDREW!) from comment #15)
> Maybe on low memory, we can kill the context and send context-lost to the
> demo?

Sure, I would support that, if we have a good way of detecting 'low memory' early enough to avoid OOM crash.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

the OS calls onLowMemory() on our activity [https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#2219]. In our handler for that we call onLowMemory() on GeckoAppShell [https://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJNI.cpp#153] which sends out a "memory-pressure" notification. You should be able to listen for that notification and recover in time.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the pointers. Doing this now. Found sample code in imgCacheObserver. This might help with bug 736436 as well.

Comment 19

[Cristian Nicolae (:xti)]

I tried to find a regression range using builds from November and December 2011, but the app will always crash. There are different signatures, depending of webgl page tested (crash example: https://crash-stats.mozilla.com/report/index/bp-2b3a9075-dac6-47a8-a6e9-d932b2120406). 

I will try to use builds since January 2012 or later.

Comment 20

[Cristian Nicolae (:xti)]

Different crashes still occur in the maple branch, so this makes really difficult to find a regression range for this issue.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: small simplification in WebGL lost context handling

This patch removes mContextLostDueToTest and the surrounding logic, as I couldn't understand its use and was unsure if that was going to interfere with the artificial context loss that I want to generate here. Please explain/comment.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: lose WebGL context on memory-pressure event

This patch intends to register an observer that listens for "memory-pressure" and upon that, loses the WebGL context.

Preliminary patch with logging to understand the following problem: while the observer is successfully registered, its Observe() function is never called.

Specifically, my logs contain

I/Gecko   ( 7673): about to register WebGLMemoryPressureObserver...
I/Gecko   ( 7673): registered WebGLMemoryPressureObserver. Success: 1.

but I never see the printf_stderr from WebGLMemoryPressureObserver::Observe. Yet, the OOM crash does happen, and shortly before that (and after the observer registration), the logs have:

I/ActivityManager(  107): Process com.android.voicedialer (pid 7637) has died.
I/ActivityManager(  107): Process com.svox.pico (pid 7645) has died.
I/ActivityManager(  107): Process com.android.quicksearchbox (pid 7653) has died.
I/ActivityManager(  107): Low Memory: No more background processes.
I/Gecko   ( 7673): [egl] > void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
I/Gecko   ( 7673): [egl] < void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
I/ActivityManager(  107): Process com.android.launcher (pid 7569) has died.
I/ActivityManager(  107): Low Memory: No more background processes.

Is there something else that I need to do to get these memory-pressure callbacks?

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

I tried leaking the observer object (by using a plain pointer and NS_ADDREF) like imgLoader.cpp does for gCacheObserver, but that didn't make a difference. It's my understanding indeed that AddObserver makes a strong reference to the observer object.

Comment 24

[Josh Matthews [:jdm]]

That looks correct to me. Maybe we don't have machinery hooked up to detect low memory and send the memory-pressure notification?

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 613150 [details] [diff] [review]
lose WebGL context on memory-pressure event

Looks ok, some nits to pick.

I don't know if the low-memory indicator you're seeing in logcat is something that we *could* register to listen in gecko, nor if we *do* register to listen for that.  dougt or blassey would know better than me.

Comment 26

[Doug Sherk (:drs) (inactive)]

Re "small simplification in WebGL lost context handling", has this been tested and/or put through try? I'm pretty sure this will mess up conformance tests.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

Not tried on tryserver yet, but the conformance tests on lose_context succeeded locally. Can you explain where you expect trouble?

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: desperate attempt at getting "low memory" event before we die

I still don't understand why we die without getting any memory-pressure notification.

This patch adds some printf_stderr to Java_org_mozilla_gecko_GeckoAppShell_onLowMemory: they are not printed in adb logcat.

This patch then adds a GeckoAppShell.geckoEventSync(); to onLowMemory() to make sure we don't just die while waiting for event delivery: makes no difference.

In fact, onLowMemory() has a logging command,

    Log.e(LOGTAG, "low memory");

and I don't see that message in adb logcat. All I can see is a different "Low Memory" message with uppercase, as in comment 22. So it seems that onLowMemory() is never called at all!

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

Just for fun, I pressed the home button while this demo was loading and logcat was logging. I got this death:

04-09 22:48:58.656 I/Gecko   ( 8947): [egl] > void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
04-09 22:48:58.656 I/Gecko   ( 8947): [egl] < void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
04-09 22:48:58.878 I/ActivityManager(  107): Process android.process.acore (pid 8916) has died.
04-09 22:48:59.191 I/Gecko   ( 8947): [gl:0x4eade000] > void mozilla::gl::GLContext::fTexImage2D(GLenum, GLint, GLint, GLsizei, GLsizei, GLint, GLenum, GLenum, const GLvoid*)
04-09 22:48:59.285 I/ActivityManager(  107): Process org.mozilla.fennec_bjacob (pid 8947) has died.
04-09 22:48:59.285 I/WindowManager(  107): WIN DEATH: Window{4086d240 org.mozilla.fennec_bjacob/org.mozilla.fennec_bjacob.App paused=false}
04-09 22:48:59.292 I/WindowManager(  107): WIN DEATH: Window{408a4ef8 SurfaceView paused=false}
04-09 22:48:59.296 I/ActivityManager(  107): Low Memory: No more background processes.

This confirms that Android just kills the background processes first, so here Fennec got killed before adb reported 'Low Memory'. We still didn't get any onLowMemory() callback.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

http://groups.google.com/group/android-developers/browse_thread/thread/1bf9e090f4ce7d1a

onLowMemory only gets called when the system-wide memory is running low.

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

But here I have the impression that it's a system-wide memory problem, not a video memory problem. A lot of the crashes occur during the texture conversions done by WebGL.texImage2D before the actual glTexImage2D call: that's plain old malloc'd or new'd buffer, not a video memory resource.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: log in Quake3 demo, showing memory-pressure observer called 1.6 second after onLowMemory(), too late to avoid the crash

So far I was testing with the testcase from bug 736436:

  http://crazybugs.ivank.net/

Now that I've actually tried with the STR in comment 0, visiting

  http://media.tojicode.com/q3bsp/

and waiting a bit (doesn't always reproduce, though), I've got my memory-pressure observer called!

But this didn't prevent the crash. Looking at the log, we see that between onLowMemory() and the actual memory-pressure observer call, 1.6 second elapses, during which we continue happily allocating WebGL resources:


04-09 23:13:17.074 E/GeckoApp( 9011): low memory
04-09 23:13:17.078 I/Gecko   ( 9011): Java_org_mozilla_gecko_GeckoAppShell_onLowMemory
04-09 23:13:17.078 I/Gecko   ( 9011): Notifying memory-pressure observers

[ ... 1.6 second elapses, lots of GL calls allocating resources ... ]

04-09 23:13:18.671 I/Gecko   ( 9011): WebGLMemoryPressureObserver::Observe: topic = memory-pressure

[ now we immediately start destroying all our WebGL resources... ]

04-09 23:13:18.671 I/Gecko   ( 9011): [egl] > void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
04-09 23:13:18.671 I/Gecko   ( 9011): [egl] < void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
04-09 23:13:18.671 I/Gecko   ( 9011): [egl] > void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
04-09 23:13:18.671 I/Gecko   ( 9011): [egl] < void* mozilla::gl::GLLibraryEGL::fGetCurrentContext()
04-09 23:13:18.671 I/Gecko   ( 9011): [gl:0x4dedc800] > void mozilla::gl::GLContext::fDeleteTextures(GLsizei, GLuint*)
04-09 23:13:18.671 I/Gecko   ( 9011): [gl:0x4dedc800] < void mozilla::gl::GLContext::fDeleteTextures(GLsizei, GLuint*) [0x0000]


If we could react faster, perhaps we could avoid crashing.

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

What is weird though in that log, is that we're getting the memory pressure callback 3 times, and destroying the WebGL context and GL context 2 times. needs investigating....

Comment 34

[Doug Sherk (:drs) (inactive)]

Comment on attachment 613148 [details] [diff] [review]
small simplification in WebGL lost context handling

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

re: context loss semantics

I can't remember why I did it that way, but looking at the code now it's plausible that it would be ok without this variable. 

Reason: when a script testing its context loss functionality invokes the loseContext() function, this method invokes the ForceLoseContext() method on the WebGLContext. ForceLoseContext() invokes DestroyResourcesAndContext(), which nulls the gl member. The first check in MaybeRestoreContext() is an escape on gl==nsnull, so it should never get beyond there to begin with.

I'd run this through a full try sweep, but I'll tentatively r+ it.

Comment 35

[Doug Sherk (:drs) (inactive)]

It won't let me r+ it for some reason (I can't modify the attachment state in any way other than publishing a review which doesn't change the status), so just take comment #34 for it.

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: lose WebGL context on memory-pressure event

Refreshed, removed debug printfs. ready for review.

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix IsContextLost checks in WebGL implementation

Many WebGL functions were not properly checking for context loss, resulting in crashes as they were trying to proceed with the already lost WebGL context.

Other WebGL functions didn't set their *retval before returning, potentially returning an uninitialized value.

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 613148 [details] [diff] [review]
small simplification in WebGL lost context handling

feedback+ to account for Doug's answer above.

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 613476 [details] [diff] [review]
desperate attempt at getting "low memory" event before we die

Patch obsolete: doesn't make a difference. We now recover from Quake 3 even without this patch, and we don't recover from CrazyBugs even with this patch.

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=6f6a18879c8c

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

With these patches, the Quake 3 demo doesn't crash anymore (it just loses the WebGL context) in a debug build of Fennec.

I could never reproduce the problem with the Quake 3 demo with an optimized, non-debug build of Fennec.

So I'll consider this bug fixed once these patch land.

Comment 42

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 614067 [details] [diff] [review]
lose WebGL context on memory-pressure event

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

Fix your indents!

::: content/canvas/src/WebGLContext.cpp
@@ +84,5 @@
> +WebGLMemoryPressureObserver::Observe(nsISupports* aSubject,
> +                                     const char* aTopic,
> +                                     const PRUnichar* aSomeData)
> +{
> +  if (strcmp(aTopic, "memory-pressure") == 0)

2-space indents.

::: content/canvas/src/WebGLContext.h
@@ +2802,5 @@
> +class WebGLMemoryPressureObserver MOZ_FINAL
> +    : public nsIObserver
> +{
> +public:
> +  NS_DECL_ISUPPORTS

2-space indents

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: remove ShouldEnableRobustnessTimer

I stumbled upon this on my Linux desktop which does not satisfy any of the conditions in ShouldEnableRobustnessTimer. Basically, if we want "memory-pressure" to always generate context{lost,restored} events (e.g. by clicking "minimize memory usage" in about:memory), we must always enable this timer. It can't be a big regression since we already enable whenever we're on ES and whenever robustness is supported.

Comment 44

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: make memory-pressure observer lifetime match WebGL context lifetime

I came across this by testing on my desktop, using about:memory "minimize memory usage". I was getting use-after-free crashes when a memory-pressure event reached an already destroyed WebGL context.

Comment 45

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: check if we still have a canvas

Last patch in my "let's try not crash when stress-testing this with about:memory buttons" frenzy. I was getting a crash when closing the WebGL page and immediately after sending a memory-pressure event, as a Notify callback came after the canvas element was gone and HTMLCanvasElement() was returning null, which we then dereferenced ( ->OwnerDoc() ).

With all these patches, I can't get a crash anymore.

Comment 46

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 614614 [details] [diff] [review]
remove ShouldEnableRobustnessTimer

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

::: content/canvas/src/WebGLContext.h
@@ -649,4 @@
>      // Sets up the GL_ARB_robustness timer if it isn't already, so that if the
>      // driver gets restarted, the context may get reset with it.
>      void SetupRobustnessTimer() {
> -        if (!ShouldEnableRobustnessTimer())

If this is going to apply in situations outside of robustness (such as memory pressure notifications), these function/variable names should probably be changed to match what they do.

Comment 47

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 614616 [details] [diff] [review]
make memory-pressure observer lifetime match WebGL context lifetime

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

::: content/canvas/src/WebGLContext.cpp
@@ +206,5 @@
> +    nsCOMPtr<nsIObserverService> observerService
> +        = mozilla::services::GetObserverService();
> +    if (observerService) {
> +        observerService->RemoveObserver(mMemoryPressureObserver,
> +                                        "memory-pressure");

mMemoryPressureObserver seems like it could be null. Is it harmless to try to remove a null observer?

::: content/canvas/src/WebGLContext.h
@@ +515,5 @@
>      bool antialias;
>      bool preserveDrawingBuffer;
>  };
>  
> +class WebGLMemoryPressureObserver MOZ_FINAL

Can we not just forward-declare this class instead of WebGLContext, and leave this class where it was? I would prefer to keep such smaller classes buried further down the file.

Comment 48

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #47)
> Comment on attachment 614616 [details] [diff] [review]
> make memory-pressure observer lifetime match WebGL context lifetime
> 
> Review of attachment 614616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +206,5 @@
> > +    nsCOMPtr<nsIObserverService> observerService
> > +        = mozilla::services::GetObserverService();
> > +    if (observerService) {
> > +        observerService->RemoveObserver(mMemoryPressureObserver,
> > +                                        "memory-pressure");
> 
> mMemoryPressureObserver seems like it could be null. Is it harmless to try
> to remove a null observer?

It can't be null: if we're here, that means that the if(!gl) early return just above didn't happen, so gl is non-null, which means that InitAndValidateShaders was run more recently than DestroyResourcesAndContext, which means that mMemoryPressureObserver is non-null.

Yes, we need to make the lifecycle more explicit.

> 
> ::: content/canvas/src/WebGLContext.h
> @@ +515,5 @@
> >      bool antialias;
> >      bool preserveDrawingBuffer;
> >  };
> >  
> > +class WebGLMemoryPressureObserver MOZ_FINAL
> 
> Can we not just forward-declare this class instead of WebGLContext, and
> leave this class where it was? I would prefer to keep such smaller classes
> buried further down the file.

You're right.

Comment 49

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: remove ShouldEnableRobustnessTimer, rename Robustness=>ContextLoss

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: make memory-pressure observer lifetime match WebGL context lifetime

Comment 51

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: check if we still have a canvas

Comment 52

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: make memory-pressure observer lifetime match WebGL context lifetime

Comment 53

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 614886 [details] [diff] [review]
check if we still have a canvas

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

::: content/canvas/src/WebGLContext.cpp
@@ +1053,5 @@
>  {
>      TerminateContextLossTimer();
> +
> +    if (!HTMLCanvasElement()) {
> +        // the canvas is gone. That happens when the page was closed before we got

Please capitalize the initial T in this comment. (biggest nit ever)

Comment 54

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 615061 [details] [diff] [review]
make memory-pressure observer lifetime match WebGL context lifetime

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

Question:

::: content/canvas/src/WebGLContext.cpp
@@ +206,5 @@
> +    nsCOMPtr<nsIObserverService> observerService
> +        = mozilla::services::GetObserverService();
> +    if (observerService) {
> +        observerService->RemoveObserver(mMemoryPressureObserver,
> +                                        "memory-pressure");

Shouldn't we be doing this even if we no longer have a GL context?

::: content/canvas/src/WebGLContext.h
@@ +101,5 @@
>  class WebGLFramebuffer;
>  class WebGLRenderbuffer;
>  class WebGLUniformLocation;
>  class WebGLExtension;
> +class WebGLContext;

I don't think this is needed anymore, but it won't hurt. It's good to be able to reference WebGLContext anywhere in its own header.

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Something else I thought of while looking at our java code: it's possible that dalvik will clear SoftReference instances in java before it terminates an activity due to memory pressure. See http://developer.android.com/reference/java/lang/ref/SoftReference.html - in particular, "all SoftReferences pointing to softly reachable objects are guaranteed to be cleared before the VM will throw an OutOfMemoryError"

You can try to use this as a memory-pressure detector by having some dummy object in Java pointed to by a SoftReference, and overriding the dummy object's finalize() method to trigger the memory-pressure detector. No idea if it'll work, but it might be worth a shot.

Comment 56

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #54)
> Comment on attachment 615061 [details] [diff] [review]
> make memory-pressure observer lifetime match WebGL context lifetime
> 
> Review of attachment 615061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Question:
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +206,5 @@
> > +    nsCOMPtr<nsIObserverService> observerService
> > +        = mozilla::services::GetObserverService();
> > +    if (observerService) {
> > +        observerService->RemoveObserver(mMemoryPressureObserver,
> > +                                        "memory-pressure");
> 
> Shouldn't we be doing this even if we no longer have a GL context?

Yes, but we only do

    gl = nsnull

right here, so this is safe. But you're right, I will make this change as it's more fool-proof and doesn't cost much.

Please just r+ as I dont have time to make the updated patch now (doesn't apply cleanly, no time right now). I'll make this change and land it asap.

Comment 57

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 615061 [details] [diff] [review]
make memory-pressure observer lifetime match WebGL context lifetime

Ok.

Comment 58

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 613148 [details] [diff] [review]
small simplification in WebGL lost context handling

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any):

Comment 59

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 614067 [details] [diff] [review]
lose WebGL context on memory-pressure event

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any):

Comment 60

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 614083 [details] [diff] [review]
Fix IsContextLost checks in WebGL implementation

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any):

Comment 61

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 614879 [details] [diff] [review]
remove ShouldEnableRobustnessTimer, rename Robustness=>ContextLoss

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any):

Comment 62

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 614886 [details] [diff] [review]
check if we still have a canvas

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any):

Comment 63

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 615061 [details] [diff] [review]
make memory-pressure observer lifetime match WebGL context lifetime

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any):

Comment 64

[Benoit Jacob [:bjacob] (mostly away)]

try: https://tbpl.mozilla.org/?tree=Try&rev=3c67a2795b1e

Comment 65

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bd7c35f57806
http://hg.mozilla.org/integration/mozilla-inbound/rev/9528f37307f3
http://hg.mozilla.org/integration/mozilla-inbound/rev/ac74798eb3d3
http://hg.mozilla.org/integration/mozilla-inbound/rev/1f0b1c587cd6
http://hg.mozilla.org/integration/mozilla-inbound/rev/e855654fef7e
http://hg.mozilla.org/integration/mozilla-inbound/rev/b8bdcf5ef5ce
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd21fdb69713

Comment 66

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd7c35f57806
https://hg.mozilla.org/mozilla-central/rev/9528f37307f3
https://hg.mozilla.org/mozilla-central/rev/ac74798eb3d3
https://hg.mozilla.org/mozilla-central/rev/1f0b1c587cd6
https://hg.mozilla.org/mozilla-central/rev/e855654fef7e
https://hg.mozilla.org/mozilla-central/rev/b8bdcf5ef5ce
https://hg.mozilla.org/mozilla-central/rev/dd21fdb69713

Comment 67

[Cristian Nicolae (:xti)]

This bug can be verified just after bug 746794 is fixed. If a webgl page is loaded, Fennec will crash.