Closed Bug 736481 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- soft

People

(Reporter: xti, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

(Whiteboard: oom)

Attachments

(6 files, 7 obsolete files)

4.12 KB, patch
jgilbert
: review+
bjacob
: feedback+
Details | Diff | Splinter Review
3.71 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
5.48 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
9.29 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.35 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
4.75 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
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.
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.
(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.
Severity: normal → critical
blocking-fennec1.0: --- → -
See also bug 723369.
Whiteboard: oom
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?
(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.
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.
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.
Also, is this a recent regression? Could you bisect it?
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.
Also, when it crashes, does this bring up the crash reporter? Do you see crash links in about:crashes?
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.
No, current trunk Fennec Native just dies without the Crash Reporter appearing. Nothing shows up in about:crashes.
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.
blocking-fennec1.0: - → ?
(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.
Summary: Fennec process is stopped when loading a WebGL page due to "Low Memory" → Fennec process is stopped when loading a WebGL page due to "Low Memory" (regression from Maple merge)
Maybe on low memory, we can kill the context and send context-lost to the demo?
Assignee: nobody → bjacob
blocking-fennec1.0: ? → -
(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.
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.
Thanks for the pointers. Doing this now. Found sample code in imgCacheObserver. This might help with bug 736436 as well.
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.
Different crashes still occur in the maple branch, so this makes really difficult to find a regression range for this issue.
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.
Attachment #613148 - Flags: review?(bugzilla)
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?
Attachment #613150 - Flags: feedback?(jones.chris.g)
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.
That looks correct to me. Maybe we don't have machinery hooked up to detect low memory and send the memory-pressure notification?
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.
Attachment #613150 - Flags: feedback?(jones.chris.g) → feedback+
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.
Not tried on tryserver yet, but the conformance tests on lose_context succeeded locally. Can you explain where you expect trouble?
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!
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.
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.
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.
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 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.
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.
Refreshed, removed debug printfs. ready for review.
Attachment #613150 - Attachment is obsolete: true
Attachment #614067 - Flags: review?(jgilbert)
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.
Attachment #614083 - Flags: review?(jgilbert)
Attachment #614083 - Flags: feedback?(bugzilla)
Attachment #613148 - Flags: review?(jgilbert)
Attachment #613148 - Flags: review?(bugzilla)
Attachment #613148 - Flags: feedback?(bugzilla)
Comment on attachment 613148 [details] [diff] [review]
small simplification in WebGL lost context handling

feedback+ to account for Doug's answer above.
Attachment #613148 - Flags: feedback?(bugzilla) → feedback+
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.
Attachment #613476 - Attachment is obsolete: true
Attachment #613480 - Attachment is obsolete: true
Blocks: 744515
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.
Attachment #614083 - Flags: review?(jgilbert) → review+
Attachment #613148 - Flags: review?(jgilbert) → review+
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
Attachment #614067 - Flags: review?(jgilbert) → review+
Keywords: qawanted
blocking-fennec1.0: - → beta+
Keywords: qawanted
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.
Attachment #614614 - Flags: review?(jgilbert)
Attachment #614614 - Flags: feedback?(bugzilla)
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.
Attachment #614616 - Flags: review?(jgilbert)
Attachment #614616 - Flags: feedback?(bugzilla)
Attached patch check if we still have a canvas (obsolete) — Splinter Review
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.
Attachment #614617 - Flags: review?(jgilbert)
Attachment #614617 - Flags: feedback?(bugzilla)
Attachment #614617 - Flags: review?(jgilbert) → review+
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.
Attachment #614614 - Flags: review?(jgilbert) → review-
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.
blocking-fennec1.0: beta+ → soft
(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.
Attachment #614614 - Attachment is obsolete: true
Attachment #614614 - Flags: feedback?(bugzilla)
Attachment #614879 - Flags: review?(jgilbert)
Attachment #614879 - Flags: feedback?(bugzilla)
Attachment #614616 - Attachment is obsolete: true
Attachment #614616 - Flags: review?(jgilbert)
Attachment #614616 - Flags: feedback?(bugzilla)
Attachment #614881 - Flags: review?(jgilbert)
Attachment #614881 - Flags: feedback?(bugzilla)
Attachment #614617 - Attachment is obsolete: true
Attachment #614617 - Flags: feedback?(bugzilla)
Attachment #614886 - Flags: review?(jgilbert)
Attachment #614886 - Flags: feedback?(bugzilla)
Attachment #614881 - Attachment is obsolete: true
Attachment #614881 - Flags: review?(jgilbert)
Attachment #614881 - Flags: feedback?(bugzilla)
Attachment #615061 - Flags: review?(jgilbert)
Attachment #615061 - Flags: feedback?(bugzilla)
Attachment #614879 - Flags: review?(jgilbert) → review+
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)
Attachment #614886 - Flags: review?(jgilbert) → review+
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.
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.
(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 on attachment 615061 [details] [diff] [review]
make memory-pressure observer lifetime match WebGL context lifetime

Ok.
Attachment #615061 - Flags: review?(jgilbert) → review+
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):
Attachment #613148 - Flags: approval-mozilla-central?
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):
Attachment #614067 - Flags: approval-mozilla-central?
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):
Attachment #614083 - Flags: feedback?(bugzilla) → approval-mozilla-central?
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):
Attachment #614879 - Flags: feedback?(bugzilla) → approval-mozilla-central?
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):
Attachment #614886 - Flags: feedback?(bugzilla) → approval-mozilla-central?
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):
Attachment #615061 - Flags: feedback?(bugzilla) → approval-mozilla-central?
Attachment #613148 - Flags: approval-mozilla-central?
Attachment #614067 - Flags: approval-mozilla-central?
Attachment #614083 - Flags: approval-mozilla-central?
Attachment #614879 - Flags: approval-mozilla-central?
Attachment #614886 - Flags: approval-mozilla-central?
Attachment #615061 - Flags: approval-mozilla-central?
This bug can be verified just after bug 746794 is fixed. If a webgl page is loaded, Fennec will crash.
Depends on: 746794
Blocks: 760415
You need to log in before you can comment on or make changes to this bug.