998916 - Defer the webgl context restore until the app becomes foreground

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

After context lost, gecko will restore the webgl context in 1 second.
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#1229

Even though the app is background, the context still restores.
At tarako device, I usually get OOM when b2g is restoring.
I'm trying to defer the restore until the app becomes foreground.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] restore_when_foreground.patch

defer the restore until the app becomes foreground.

Comment 2

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

Comment on attachment 8409626 [details] [diff] [review]
[WIP] restore_when_foreground.patch

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

This is only an early feedback+; we probably need at least one more round of more precise feedback.

The general intent and implementation approach sound good. However, I can't comment very precisely because I don't know exactly what precise behavior you want to implement. I need you to comment on exactly what behavior you want to implement, before I can say for sure whether this patch is a good approach to implementing it.

Also, it seems that you're always registering an observer for process-priority-change, but that observer is only ever needed while the context is lost. Could you only observe process-priority-change when the context is lost, so that we limit the number of unnecessary live observers?

Also, I would suggest looping in :jgilbert early on this. Consider him the primary reviewer for this kind of WebGL patches :-)

Comment 3

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

(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 8409626 [details] [diff] [review]
> [WIP] restore_when_foreground.patch
> 
> Review of attachment 8409626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is only an early feedback+; we probably need at least one more round of
> more precise feedback.
> 
> The general intent and implementation approach sound good. However, I can't
> comment very precisely because I don't know exactly what precise behavior
> you want to implement. I need you to comment on exactly what behavior you
> want to implement, before I can say for sure whether this patch is a good
> approach to implementing it.

More precisely, here is an example of something that I need to know:

Do you want to keep unchanged the logic to decide whether to lose the context on memory-pressure? If yes, then can you explain the reason for the change of "heap-minimize" to "low-memory" ?

Thanks!

Comment 4

[Kelsey Gilbert [:jgilbert]]

(This should probably be rebased on bug 980178)

I think we need more thought on this. (Design review!)

We need to decide:
1. When we're going to ForceLoseContext
2. When we're going to trigger webglcontextrestored.
3. If we want to trigger webglcontextrestored at a different time than the WebGL spec says to, bring that up with the WG.

So far:
1. When we get heap-minimize
2. Enqueued after the context is lost.
3. This is the current spec.

It sounds like what you want:
1. When we get low-memory, not heap-minimize
2. Sent only after the app is in the foreground.
3. I think this is technically non-spec, so we'd have to get that changed.

I believe the proper way to do this with the current web primitives is to have the app handle context-loss, but not try to restore the context until it regains visibility. This is also true for WebGL context embedded in scrolling pages, or background tabs.

However, I think that there's not much reason for webglcontextrestored at present, since it's the same as Post()ing a message yourself when you get a context-lost event. Instead, I think we should use webglcontextrestored to indicate that "you *should* restore now (since you're becoming visible)".

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp?from=webglcontext.cpp#(In reply to Benoit Jacob [:bjacob] from comment #3)
> (In reply to Benoit Jacob [:bjacob] from comment #2)
> More precisely, here is an example of something that I need to know:
> 
> Do you want to keep unchanged the logic to decide whether to lose the
> context on memory-pressure? If yes, then can you explain the reason for the
> change of "heap-minimize" to "low-memory" ?
> 

We send the low-memory message instead of heap-minimize now.
http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#1059

So in these codes, if we receive "low-memory" and the app is backgroud, we will always lose the context.
The "wantToLoseContext" will be true in this case.
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp?from=webglcontext.cpp#87

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

The spec say we should queue a task after lost.
http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2

I want to do:
1. receive the memory-pressure event(listen heap-minimize before, but low-memory in mozilla-centrol now)
2. lose the context
3. if app prevents default behavior, defer the restore task <= Can we call this step to be "queue a task"?
4. if app becomes foreground, process the restore task.

> I believe the proper way to do this with the current web primitives is to
> have the app handle context-loss, but not try to restore the context until
> it regains visibility. This is also true for WebGL context embedded in
> scrolling pages, or background tabs.
Do you mean we should listen both visibilityChange and webglcontextrestored in app?
If app receives webglcontextrestored, it should not do the restore until the document.hiden is false. Right?
The app will become more complex and we still alloc more memory for the webgl context when the app is background.

> However, I think that there's not much reason for webglcontextrestored at
> present, since it's the same as Post()ing a message yourself when you get a
> context-lost event. Instead, I think we should use webglcontextrestored to
> indicate that "you *should* restore now (since you're becoming visible)".
Sorry, Jeff. I can't get your idea.
The app still restores when it receives webglcontextrestored event.
And we send the webglcontextrestored just at foreground in this model.

Comment 7

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

(In reply to Jerry Shih[:jerry] from comment #5)
> (In reply to Benoit Jacob [:bjacob] from comment #3)
> > Do you want to keep unchanged the logic to decide whether to lose the
> > context on memory-pressure? If yes, then can you explain the reason for the
> > change of "heap-minimize" to "low-memory" ?
> 
> We send the low-memory message instead of heap-minimize now.
> http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.
> cpp#1059

Ah, thanks, this clears up this confusion.

Comment 8

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

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> (This should probably be rebased on bug 980178)
> 
> I think we need more thought on this. (Design review!)
> 
> We need to decide:
> 1. When we're going to ForceLoseContext
> 2. When we're going to trigger webglcontextrestored.
> 3. If we want to trigger webglcontextrestored at a different time than the
> WebGL spec says to, bring that up with the WG.
> 
> So far:
> 1. When we get heap-minimize
> 2. Enqueued after the context is lost.
> 3. This is the current spec.
> 
> It sounds like what you want:
> 1. When we get low-memory, not heap-minimize
> 2. Sent only after the app is in the foreground.
> 3. I think this is technically non-spec, so we'd have to get that changed.

I don't think that the spec can say anything so precise about when context restoration can happen. This has got to be left up to the implementation, which plays in our favor here.

And indeed, http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2 just says that the implementation should "Await a restorable drawing buffer". Whatever that means.

So I don't think that we'll need any spec change here.

> 
> I believe the proper way to do this with the current web primitives is to
> have the app handle context-loss, but not try to restore the context until
> it regains visibility. This is also true for WebGL context embedded in
> scrolling pages, or background tabs.

Oh, that is a really good point. Content that is unvisible because of scrolling or other reasons, is another good opportunity to defer context restoration and save resources. I could easily imagine a tall page with many WebGL contexts, hitting the limit of contexts per principal. To handle that well, it would be very useful to avoid restoring scrolled-away contexts.

> 
> However, I think that there's not much reason for webglcontextrestored at
> present, since it's the same as Post()ing a message yourself when you get a
> context-lost event. Instead, I think we should use webglcontextrestored to
> indicate that "you *should* restore now (since you're becoming visible)".

I don't understand what you meant here: the point of webglcontextrestored is that it is the only way for applications to know when their WebGL contexts have been restored i.e. when is a good time to resume making WebGL calls.

Comment 9

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

Jerry: as discussed above, I think that Jeff has a really good point about visibility: what you really want to observe is not really the changes in process priority, it is whether the content is visible. "Is the app background/foreground?" is only a special case, and the more interesting, more general thing to track is "Is this stuff visible?". I don't know what is the specific kind of event that you need of observe here, but with some luck, "visibility change" is exactly it? I suppose that you know much better than we do about this ;-)

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

If we restore the context when the canvas is really visible(in the screen area), people maybe feel jackey when it restores. 

If my idea does not break the spec, I will try to use the visibility event of app first. All contexts in page will call restore when the tab/app becomes visible.

reference for visibilityChange event:
https://developer.mozilla.org/zh-TW/docs/Web/Reference/Events/visibilitychange
.

Comment 11

[Kelsey Gilbert [:jgilbert]]

For our own apps, we can handle this properly:
In general:
1. Override the default context-loss handler
2. On context-lost event, note that we're lost.
3. On visibility change-to-visible, ask for the context to be restored.

I don't think the spec is as flexible as you say, unless we don't give events to apps in the background, which seems overly restrictive. 'Queue an event' (I believe) means drop the event in the message queue for the document(?).

FWIW, I think we should change this, so that the implementation is free to pick a different time to restore the context, if auto-context-restore is not disabled. (via preventDefault)

(In reply to Jerry Shih[:jerry] from comment #10)
> If we restore the context when the canvas is really visible(in the screen
> area), people maybe feel jackey when it restores. 
This will also be true for full-screen apps, or apps where being restored to the foreground means they also become visible.

In short:
I think we should change our default context-loss handler so that it requests context-restore when the canvas is next visible. (Maybe immediately, for the current page)

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> For our own apps, we can handle this properly:
> In general:
> 1. Override the default context-loss handler
> 2. On context-lost event, note that we're lost.
> 3. On visibility change-to-visible, ask for the context to be restored.

How can we ask for restoring context?
I'm trying to do this before, but I can't request the restoring in gaia.
We can't use restoreContext() extension.
The restoreContext() only can restore the context lost by loseContext().
http://www.khronos.org/registry/webgl/extensions/WEBGL_lose_context/

Comment 13

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] from comment #12)
> (In reply to Jeff Gilbert [:jgilbert] from comment #11)
> > For our own apps, we can handle this properly:
> > In general:
> > 1. Override the default context-loss handler
> > 2. On context-lost event, note that we're lost.
> > 3. On visibility change-to-visible, ask for the context to be restored.
> 
> How can we ask for restoring context?
> I'm trying to do this before, but I can't request the restoring in gaia.
> We can't use restoreContext() extension.
> The restoreContext() only can restore the context lost by loseContext().
> http://www.khronos.org/registry/webgl/extensions/WEBGL_lose_context/

Ah, ok, so the specific language here is 'simulate the context being restored so as to trigger the steps described in the WebGL spec'. I'll ask the WG about this, but I think we should be able to use it for our usecase here. Specifically, I think 'simulate' refers to us not actually restoring the context at the GL level, but rather restoring it at the WebGL level. (simulating a GL-level context-restore)

That said, we can still do exactly as I said, except instead of calling restore(), just delete the canvas, create a new canvas, and create a new WebGL context from that canvas. This is essentially what we do on context restoration anyways.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8409626 [details] [diff] [review]
[WIP] restore_when_foreground.patch

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

Ok, so since bug 800166, we've been force-losing webgl contexts every time immediately as the process goes to the background. I do not think this makes for a good user experience, so I filed bug 999878.

We need to be clear here about what we're trying to fix. Is there not a way to detect that OOM occurred during context-restoration? If there is, we should force-lose that context again, and forbid it from recreating contexts.
If we're really trying to fix a bug arising from how our own apps work, we should just fix the apps. For apps with large webgl objects, the app should only re-upload it's bare-required data in the context-restored event. It should wait on visibility change to upload more expensive items.

This patch is very reliant on exactly what we do right now, with the logic split in two places. (WebGL code for 'foregrounding' and process priority code for 'backgrounding')

If we want the context to remain lost while the canvas backgrounded, we should do that explicitly. We should add observable events for 'backgrounded' and 'foregrounded', and act on those.

::: content/canvas/src/WebGLContext.cpp
@@ +71,5 @@
>  using namespace mozilla::gfx;
>  using namespace mozilla::gl;
>  using namespace mozilla::layers;
>  
> +WebGLMemoryPressureObserver::WebGLMemoryPressureObserver(WebGLContext *context)

This class is named incorrectly if we add anything other than observing memory pressure. Just `WebGLObserver` might be best.

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> That said, we can still do exactly as I said, except instead of calling
> restore(), just delete the canvas, create a new canvas, and create a new
> WebGL context from that canvas. This is essentially what we do on context
> restoration anyways.

Delete the old canvas and then create a new one. It's not easy to do for every app.
For example, in gallery, the canvas is passing from other module and we use the canvas to do some webgl stuff.
It's hard to detach the canvas and attach a new one to dom in your module. You need to add webgl context related logic to other module. If we use the original mechanism, we can handle all webgl stuff in our own module. The code is much clear.

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > That said, we can still do exactly as I said, except instead of calling
> > restore(), just delete the canvas, create a new canvas, and create a new
> > WebGL context from that canvas. This is essentially what we do on context
> > restoration anyways.
> 
> Delete the old canvas and then create a new one. It's not easy to do for
> every app.
> For example, in gallery, the canvas is passing from other module and we use
> the canvas to do some webgl stuff.
> It's hard to detach the canvas and attach a new one to dom in your module.
> You need to add webgl context related logic to other module. If we use the
> original mechanism, we can handle all webgl stuff in our own module. The
> code is much clear.

Alright, I'll ask the WG about what is meant by 'simulates'.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] restore_when_visible.patch

check visibilityChange event to restore context

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8413690 [details] [diff] [review]
[WIP] restore_when_visible.patch

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

::: content/canvas/src/WebGLContext.cpp
@@ +169,3 @@
>          wantToLoseContext = false;
> +    } else {
> +        wantToLoseContext = mContext->mLoseContextOnMemoryPressure;

B2G changes from heap-minimize to low-memory event.
How about just having one config "LoseContextOnMemoryPressure" to handle all type?

@@ +1332,5 @@
>          // (unusable).
>          if (!defaultAction && mAllowRestore) {
> +            // If we set mRestoreOnForeground, defer the restore until the app
> +            // becomes foreground.
> +            if (!mRestoreOnForeground) {

queue a restore task or wait a visibility change event to restore

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8413690 [details] [diff] [review]
[WIP] restore_when_visible.patch

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

::: content/canvas/src/WebGLContext.cpp
@@ +70,5 @@
>  using namespace mozilla::gfx;
>  using namespace mozilla::gl;
>  using namespace mozilla::layers;
>  
> +WebGLObserver::WebGLObserver(WebGLContext *context)

Star next to the type.

@@ +81,5 @@
> +
> +WebGLObserver::~WebGLObserver()
> +{
> +    if (mWaitMemoryPressure) {
> +        UnregisterMemoryPressureEvent();

There's no need to make this conditional, since RemoveObserver works fine with already-removed or never-added elements. Just remove it unconditionally, unless there's another reason for being more careful about this.

@@ +84,5 @@
> +    if (mWaitMemoryPressure) {
> +        UnregisterMemoryPressureEvent();
> +    }
> +    if (mWaitVisibilityChange) {
> +        UnregisterVisibilityChangeEvent();

I don't know about for RemoveSystemEventListener, but if it supports unconditionally removing, we shouldn't have this be conditional either.

@@ +157,1 @@
>                                       const char* aTopic,

Realign these indentations.

@@ +165,4 @@
>  
>      bool wantToLoseContext = true;
>  
> +    if (!mContext->mCanLoseContextInForeground && ProcessPriorityManager::CurrentProcessIsForeground()) {

Put this back on two lines, and drop the '{' to the next line, since it's a multi-line conditional, then.
As:
if (!Foo &&
    Bar())
{

@@ +179,5 @@
>      return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +WebGLObserver::HandleEvent(nsIDOMEvent *aEvent)

Star to left.

@@ +1441,5 @@
>  WebGLContext::ForceRestoreContext()
>  {
>      mContextStatus = ContextLostAwaitingRestore;
> +    // Queue up a task to restore the event.
> +    SetupContextLossTimer();;

Double-semicolon.

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: part 1: calling webglext loseContext() will remain lost until restoreContext() is called

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] part 2: restore webgl context when visible

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jerry Shih[:jerry] from comment #20)
> Created attachment 8414433 [details] [diff] [review]
> part 1: calling webglext loseContext() will remain lost until
> restoreContext() is called

http://www.khronos.org/registry/webgl/extensions/WEBGL_lose_context/

void loseContext()
The context will remain in the lost state according to the WebGL specification until restoreContext() is called.

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] part 2: restore webgl context when visible. v2

nsIDocument::Hidden() to restore

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] part 2: restore webgl context when visible. v3

rename WebGLObserver object

Comment 25

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8417218 [details] [diff] [review]
[WIP] part 2: restore webgl context when visible. v3

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

::: content/canvas/src/WebGLContext.cpp
@@ +116,5 @@
> +            do_QueryInterface(canvasElement->OwnerDoc());
> +
> +        NS_WARN_IF_FALSE(target,
> +            "WebGL DOMEventTarget is null. Can't unregister visibilitychange event");
> +        target->RemoveSystemEventListener(NS_LITERAL_STRING("visibilitychange"),

http://dxr.mozilla.org/mozilla-central/source/dom/events/EventListenerManager.cpp?from=EventListenerManager.cpp&case=true#458

It seems that we can call RemoveSystemEventListener even we haven't registered before.

::: modules/libpref/src/init/all.js
@@ +3636,5 @@
>  pref("webgl.msaa-force", false);
>  pref("webgl.prefer-16bpp", false);
>  pref("webgl.default-no-alpha", false);
>  pref("webgl.force-layers-readback", false);
> +pref("webgl.lose-context-on-memory-preasure", false);

lose context when b2g receives memory-preasure event(ex. low-memory or heap-minimize)

@@ +3641,2 @@
>  pref("webgl.can-lose-context-in-foreground", true);
> +pref("webgl.restore-context-when-visible", true);

restore context when the app "document.hidden" is false

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8414433 [details] [diff] [review]
part 1: calling webglext loseContext() will remain lost until restoreContext() is called

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

::: content/canvas/src/WebGLContext.cpp
@@ +1324,5 @@
>  void
>  WebGLContext::ForceRestoreContext()
>  {
> +    if (mContextStatus == ContextLostAwaitingRestore)
> +        return;

A good protection to avoid unnecessary timer events.

Comment 27

[Boris Chiou [:boris]]

Comment on attachment 8417218 [details] [diff] [review]
[WIP] part 2: restore webgl context when visible. v3

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

Overall logic is OK to me, but there are a redundant space and a typo. Please check the inline comments. Thanks.

::: content/canvas/src/WebGLContext.cpp
@@ +282,3 @@
>  
> +    mContextObserver = new WebGLObserver(this);
> + 

redundant space

@@ +1322,5 @@
>          // If the script handled the event and we are allowing restores, then
>          // mark it to be restored. Otherwise, leave it as context lost
>          // (unusable).
>          if (!defaultAction && mAllowRestore) {
> +            // If we set mRestoreOnForeground, defer the restore until the app

I think you want to say "mRestoreWhenVisible", right?

Comment 28

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: part 2: restore webgl context when visible. v4

update patch for comment 27

Comment 29

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8418577 [details] [diff] [review]
part 2: restore webgl context when visible. v4

Hi Olli,

I use AddSystemEventListener and RemoveSystemEventListener.
Is it safe to call AddSystemEventListener with the same event for several times?
And is it safe to call RemoveSystemEventListener with the unregistered event name?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jerry Shih[:jerry] from comment #29)
> Comment on attachment 8418577 [details] [diff] [review]
> part 2: restore webgl context when visible. v4
> 
> Hi Olli,
> 
> I use AddSystemEventListener and RemoveSystemEventListener.
> Is it safe to call AddSystemEventListener with the same event for several
> times?
So if some event listener has been added for event type 'foo' already, calling 
add(System)EventListener  using 'foo' doesn't add the listener again.
But be careful adding the same _listener_. Not some new instance of a listener.



> And is it safe to call RemoveSystemEventListener with the unregistered event
> name?
This is ok. If there is no listener for the event type, the call is just no-op

...looking at the patch...

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8418577 [details] [diff] [review]
part 2: restore webgl context when visible. v4

(I don't know why this code isn't using Mozilla coding style for indentation, but 4 spaces. However since the old code isn't following it, I guess no need to change that.)

In general methods taking no params aren't declared FooBar(void), but just FooBar()

>+WebGLObserver::~WebGLObserver()
>+{
>+    UnregisterMemoryPressureEvent();
>+    UnregisterVisibilityChangeEvent();
These look useless. WebGLObserver can't be deleted if it is registered as an event listener or strong
notification observer. 

>+}
>+
>+void
>+WebGLObserver::RegisterVisibilityChangeEvent(void)
>+{
>+    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
>+
>+    NS_WARN_IF_FALSE(canvasElement,
>+        "WebGL HTMLCanvasElement is null. Can't register visibilitychange event");
>+    if (canvasElement) {
>+        nsCOMPtr<nsIDOMEventTarget> target =
>+            do_QueryInterface(canvasElement->OwnerDoc());
OwnerDoc() is an EventTarget, no need to QI.
nsIDocument inherits nsINode, which inherits EventTarget, which inherits nsIDOMEventTarget

>+WebGLObserver::Observe(nsISupports* aSubject,
>+                       const char* aTopic,
>+                       const char16_t* aSomeData)
> {
>-    if (strcmp(aTopic, "memory-pressure"))
>+    if (strcmp(aTopic, "memory-pressure")) {
>         return NS_OK;
>+    }
> 
>     bool wantToLoseContext = true;
> 
>     if (!mContext->mCanLoseContextInForeground &&
>-        ProcessPriorityManager::CurrentProcessIsForeground())
>+            ProcessPriorityManager::CurrentProcessIsForeground()) {
Why this indentation change?

>+class WebGLObserver MOZ_FINAL

> private:
>-  WebGLContext *mContext;
>+    WebGLContext* mContext;
What guarantees that mContext doesn't ever point to a deleted object?
What guarantees that WebGLContext outlives the WebGLObserver which is registered as an event listener on
some <canvas> element? Either fix or explain. Because of this, r-. So either a new patch or explanation
why it can't point to a deleted object (and then re-ask review)

Comment 32

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Olli Pettay [:smaug] from comment #31)
> Comment on attachment 8418577 [details] [diff] [review]
> part 2: restore webgl context when visible. v4
> >+WebGLObserver::~WebGLObserver()
> >+{
> >+    UnregisterMemoryPressureEvent();
> >+    UnregisterVisibilityChangeEvent();
> These look useless. WebGLObserver can't be deleted if it is registered as an
> event listener or strong
> notification observer. 

Do you mean the observer service will hold a reference to WebGLObserver after register?
So the WebGLObserver destructor will be called when: 1.WebGLContext doesn't hold the WebGLObserver. 2. we call unregister event outside. Thus, the unregister in destructor is no used. Is this description correct?

> >     if (!mContext->mCanLoseContextInForeground &&
> >-        ProcessPriorityManager::CurrentProcessIsForeground())
> >+            ProcessPriorityManager::CurrentProcessIsForeground()) {
> Why this indentation change?

case 1:
if (fooooooooooooooooo0() &&
    fooooooooooooooooo1()) {
    fooooooooooooooooo2();
}

case 2:
if (fooooooooooooooooo0() &&
        fooooooooooooooooo1()) {
    fooooooooooooooooo2();
}

I don't find this case in mdn c++ coding style page, but I think case 2 is much clear to separate the expression in if and in if body.
If we have the rule for this, I just follow. 

> >+class WebGLObserver MOZ_FINAL
> 
> > private:
> >-  WebGLContext *mContext;
> >+    WebGLContext* mContext;
> What guarantees that mContext doesn't ever point to a deleted object?
> What guarantees that WebGLContext outlives the WebGLObserver which is
> registered as an event listener on
> some <canvas> element? Either fix or explain. Because of this, r-. So either
> a new patch or explanation
> why it can't point to a deleted object (and then re-ask review)

Ohh, because the unregister is useless in ~WebGLObserver(), I need to call unregister explicitly.
How about this WebGLContext destructor modification?
Thus, we can make sure that the webgl context outlives observer.

~WebGLContext()
{
  if(mWebGLObserver){
    mWebGLObserver->unregisterEvent();
  }
  ....
}

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jerry Shih[:jerry] from comment #32)
> (In reply to Olli Pettay [:smaug] from comment #31)
> > Comment on attachment 8418577 [details] [diff] [review]
> > part 2: restore webgl context when visible. v4
> > >+WebGLObserver::~WebGLObserver()
> > >+{
> > >+    UnregisterMemoryPressureEvent();
> > >+    UnregisterVisibilityChangeEvent();
> > These look useless. WebGLObserver can't be deleted if it is registered as an
> > event listener or strong
> > notification observer. 
> 
> Do you mean the observer service will hold a reference to WebGLObserver
> after register?
> So the WebGLObserver destructor will be called when: 1.WebGLContext doesn't
> hold the WebGLObserver. 2. we call unregister event outside. Thus, the
> unregister in destructor is no used. Is this description correct?
WebGLObserver dtor is called when refcnt drops to zero. And using it as an event listener makes
the event target to hold strong reference (calls AddRefs) to it.
And registering strong observer (the last param is false) makes observer service keep strong
reference to the object. 

> I don't find this case in mdn c++ coding style page, but I think case 2 is
> much clear to separate the expression in if and in if body.
> If we have the rule for this, I just follow. 
1 is the style, but apparently MDN misses that case.

> Ohh, because the unregister is useless in ~WebGLObserver(), I need to call
> unregister explicitly.
> How about this WebGLContext destructor modification?
> Thus, we can make sure that the webgl context outlives observer.
Yeah, it sounds ok if WebGLContext unregisters WebGLObserver from event target and
observer service

Comment 34

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: part 2: restore webgl context when visible. v5

update patch for comment 31

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8419169 [details] [diff] [review]
part 2: restore webgl context when visible. v5

>+WebGLObserver::WebGLObserver(WebGLContext* aContext)
>+    : mContext(aContext)
>+{
>+    //empty
>+}
>+
>+WebGLObserver::~WebGLObserver()
>+{
>+    //empty
>+}

No need for //empty comments


>+WebGLObserver::RegisterVisibilityChangeEvent()
>+{
>+    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
>+
>+    NS_WARN_IF_FALSE(canvasElement,
>+        "WebGL HTMLCanvasElement is null. Can't register visibilitychange event");
>+    if (canvasElement) {
>+        nsIDocument* document = canvasElement->OwnerDoc();
>+
>+        NS_WARN_IF_FALSE(document,
>+            "WebGL Document is null. Can't register visibilitychange event");
OwnerDoc() returns always non-null, so remove this.


>+WebGLObserver::UnregisterVisibilityChangeEvent()
>+{
>+    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
>+
>+    NS_WARN_IF_FALSE(canvasElement,
>+        "WebGL HTMLCanvasElement is null. Can't unregister visibilitychange event");
>+    if (canvasElement) {
>+        nsIDocument* document = canvasElement->OwnerDoc();
>+
>+        NS_WARN_IF_FALSE(document,
>+            "WebGL Document is null. Can't unregister visibilitychange event");
ditto

>+WebGLObserver::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+    nsAutoString type;
>+    aEvent->GetType(type);
>+    if (!type.EqualsLiteral("visibilitychange")) {
>+        return NS_OK;
>+    }
>+
>+    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
>+
>+    NS_WARN_IF_FALSE(canvasElement,
>+        "WebGL HTMLCanvasElement is null. Can't restore");
>+    if (canvasElement) {
>+        if (!canvasElement->OwnerDoc()->Hidden()) {
merge the ifs

> WebGLContext::~WebGLContext()
> {
>+    if (mContextObserver) {
>+        mContextObserver->UnregisterMemoryPressureEvent();
>+        mContextObserver->UnregisterVisibilityChangeEvent();
Could you add some Disconnect() method to WebGLObserver.
That would then call UnregisterMemoryPressureEvent and UnregisterVisibilityChangeEvent
and set mContext null.

> WebGLContext::ForceRestoreContext()
> {
>     if (mContextStatus == ContextLostAwaitingRestore)
>         return;
> 
>     mContextStatus = ContextLostAwaitingRestore;
> 
>     // Queue up a task to restore the event.
>     SetupContextLossTimer();
>+
>+    if (mContextObserver) {
Is mContextObserver ever null here?

>-class WebGLMemoryPressureObserver MOZ_FINAL
>+// Listen visibilitychange and memory-pressure event for context lose/restore
>+class WebGLObserver MOZ_FINAL
>     : public nsIObserver
>+    , public nsIDOMEventListener
> {
> public:
>-  NS_DECL_ISUPPORTS
>-  NS_DECL_NSIOBSERVER
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSIOBSERVER
>+    NS_DECL_NSIDOMEVENTLISTENER
> 
>-  WebGLMemoryPressureObserver(WebGLContext *context)
>-    : mContext(context)
>-  {}
>+    WebGLObserver(WebGLContext* aContext);
>+    ~WebGLObserver();
>+
>+    void RegisterVisibilityChangeEvent();
>+    void UnregisterVisibilityChangeEvent();
>+
>+    void RegisterMemoryPressureEvent();
>+    void UnregisterMemoryPressureEvent();
> 
> private:
>-  WebGLContext *mContext;
>+    WebGLContext* mContext;
> };
Mozilla C++ coding style uses 2 spaces for indentation, so at least don't change 2 to 4 spaces.
(But no need to fix the rest of the file to use 2 spaces)

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

jgilbert, I didn't review that from gfx point of view.

Comment 38

[Kelsey Gilbert [:jgilbert]]

(In reply to Olli Pettay [:smaug] from comment #35)
> Comment on attachment 8419169 [details] [diff] [review]
> part 2: restore webgl context when visible. v5
> 
> >-class WebGLMemoryPressureObserver MOZ_FINAL
> >+// Listen visibilitychange and memory-pressure event for context lose/restore
> >+class WebGLObserver MOZ_FINAL
> >     : public nsIObserver
> >+    , public nsIDOMEventListener
> > {
> > public:
> >-  NS_DECL_ISUPPORTS
> >-  NS_DECL_NSIOBSERVER
> >+    NS_DECL_ISUPPORTS
> >+    NS_DECL_NSIOBSERVER
> >+    NS_DECL_NSIDOMEVENTLISTENER
> > 
> >-  WebGLMemoryPressureObserver(WebGLContext *context)
> >-    : mContext(context)
> >-  {}
> >+    WebGLObserver(WebGLContext* aContext);
> >+    ~WebGLObserver();
> >+
> >+    void RegisterVisibilityChangeEvent();
> >+    void UnregisterVisibilityChangeEvent();
> >+
> >+    void RegisterMemoryPressureEvent();
> >+    void UnregisterMemoryPressureEvent();
> > 
> > private:
> >-  WebGLContext *mContext;
> >+    WebGLContext* mContext;
> > };
> Mozilla C++ coding style uses 2 spaces for indentation, so at least don't
> change 2 to 4 spaces.
> (But no need to fix the rest of the file to use 2 spaces)
The coding style mentions that it yields (in general) to the style of the surrounding code. That is, if one line out of many has a different style than the surrounding code, we should generally change it to match, even if the differing style is 'accidentally more moz-style-correct'.

Comment 39

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8414433 [details] [diff] [review]
part 1: calling webglext loseContext() will remain lost until restoreContext() is called

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

This looks fine, but I'm pretty sure I did this in the cleanup bug this bug depends on. Please rebase on that so we're not changing functionality before doing cleanup.

Comment 40

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8419169 [details] [diff] [review]
part 2: restore webgl context when visible. v5

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

Good, but some miscellaneous things left. Let's get this rebased as well.

::: content/canvas/src/WebGLContext.cpp
@@ +88,5 @@
> +{
> +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> +
> +    NS_WARN_IF_FALSE(canvasElement,
> +        "WebGL HTMLCanvasElement is null. Can't register visibilitychange event");

Use MOZ_ASSERT.

Generally the format is:
FooT* bar = FallibleGetBar();
MOZ_ASSERT(bar);
<empty line>
MoreCodeAssumingBarNonNull(bar);

If assuming `bar` non-null could be dangerous, and is not very-very unlikely to be missed by our tests, then do like:
MOZ_ASSERT(bar)
if (!bar)
  return something safe
// Ok, now assume `bar` true.

@@ +157,5 @@
>  
>      bool wantToLoseContext = true;
>  
>      if (!mContext->mCanLoseContextInForeground &&
> +        ProcessPriorityManager::CurrentProcessIsForeground()) {

Drop the { to the next line for multi-line conditionals.

@@ +162,3 @@
>          wantToLoseContext = false;
> +    } else {
> +        wantToLoseContext = mContext->mLoseContextOnMemoryPressure;

Since this is else is unconditional, just initially set wantToLoseContext to this.

@@ +1327,5 @@
> +            // If we set mRestoreWhenVisible, defer the restore until the app
> +            // becomes visible.
> +            if (!mRestoreWhenVisible && !mWaitRestoreContextExt) {
> +                // Restart the timer so that it will be restored on the next
> +                // callback.

I think this is the stuff from the old code, not yet rebased on the cleanup bug.

@@ +1423,5 @@
>      // Queue up a task to restore the event.
>      SetupContextLossTimer();
>      DestroyResourcesAndContext();
> +
> +    if (mRestoreWhenVisible && !mWaitRestoreContextExt && mContextObserver) {

Isn't mContextObserver always true? Just assert it, don't branch on it.

::: content/canvas/src/WebGLContext.h
@@ +1312,3 @@
>  
>  private:
> +    WebGLContext* mContext;

WebGLContext* const mContext;

::: content/canvas/src/WebGLContextLossTimer.cpp
@@ +25,5 @@
>  
>      mContextRestorer->InitWithFuncCallback(RobustnessTimerCallbackStatic,
> +                                           static_cast<void*>(this),
> +                                           1000,
> +                                           nsITimer::TYPE_ONE_SHOT);

Thanks for the cleanup!

Comment 41

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8414433 [details] [diff] [review]
part 1: calling webglext loseContext() will remain lost until restoreContext() is called

This part is already in Bug 980178.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: restore webgl context when visible. v6

Comment 43

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #40)
> Comment on attachment 8419169 [details] [diff] [review]
> part 2: restore webgl context when visible. v5
> ::: content/canvas/src/WebGLContext.cpp
> @@ +88,5 @@
> > +{
> > +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> > +
> > +    NS_WARN_IF_FALSE(canvasElement,
> > +        "WebGL HTMLCanvasElement is null. Can't register visibilitychange event");
> 
> Use MOZ_ASSERT.
> 
> Generally the format is:
> FooT* bar = FallibleGetBar();
> MOZ_ASSERT(bar);
> <empty line>
> MoreCodeAssumingBarNonNull(bar);
> 
> If assuming `bar` non-null could be dangerous, and is not very-very unlikely
> to be missed by our tests, then do like:
> MOZ_ASSERT(bar)
> if (!bar)
>   return something safe
> // Ok, now assume `bar` true.

I perfer use the warnning message.
Do we never get the same situation as [1]?

[1]
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#1203

Comment 44

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] from comment #43)
> (In reply to Jeff Gilbert [:jgilbert] from comment #40)
> > Comment on attachment 8419169 [details] [diff] [review]
> > part 2: restore webgl context when visible. v5
> > ::: content/canvas/src/WebGLContext.cpp
> > @@ +88,5 @@
> > > +{
> > > +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> > > +
> > > +    NS_WARN_IF_FALSE(canvasElement,
> > > +        "WebGL HTMLCanvasElement is null. Can't register visibilitychange event");
> > 
> > Use MOZ_ASSERT.
> > 
> > Generally the format is:
> > FooT* bar = FallibleGetBar();
> > MOZ_ASSERT(bar);
> > <empty line>
> > MoreCodeAssumingBarNonNull(bar);
> > 
> > If assuming `bar` non-null could be dangerous, and is not very-very unlikely
> > to be missed by our tests, then do like:
> > MOZ_ASSERT(bar)
> > if (!bar)
> >   return something safe
> > // Ok, now assume `bar` true.
> 
> I perfer use the warnning message.
> Do we never get the same situation as [1]?
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/
> WebGLContext.cpp#1203

Warnings are basically useless, since we don't really check them. Asserts is the way to be sure we're not messing up. Switch this to an assert, and if it's something we should be worried about possibly hitting, give it a safe return value in non-DEBUG code.

Don't accept bad coding practices just because they already made it into the tree.

I'm not entirely sure what you're pointing out here. Is it that we don't check the retval of MakeCurrent?

Comment 45

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8427624 [details] [diff] [review]
restore webgl context when visible. v6

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

Cool, almost there.
Don't merely WARN and then rely on the warning as a precondition. Use MOZ_ASSERT for this. If the assert has any reasonable chance of being hit (well, skipped) on non-debug builds, make sure we fail as gracefully as we can. (Sometimes MOZ_CRASH is as graceful as is safe)

::: content/canvas/src/WebGLContext.cpp
@@ +74,5 @@
>  
> +WebGLObserver::WebGLObserver(WebGLContext* aContext)
> +    : mContext(aContext)
> +{
> +

Don't leave empty lines between the brackets.

@@ +159,5 @@
> +    nsCOMPtr<nsIObserverService> observerService =
> +        mozilla::services::GetObserverService();
> +
> +    NS_WARN_IF_FALSE(observerService,
> +        "WebGL ObserverService is null. Can't unregister memory-pressure event");

When could this be true, such that we shouldn't MOZ_ASSERT it? Mention why we can't ASSERT this in a comment.

Align the second arg with the first arg. If the line is too long, wrap it like:
Foo(bar,
    "baz"
    " bat."); <- this line can have an extra indent if it needs one for readability.

@@ +175,4 @@
>          return NS_OK;
> +    }
> +
> +    bool wantToLoseContext = mContext->mLoseContextOnMemoryPressure;

MOZ_ASSERT(mContext);

@@ +201,5 @@
> +
> +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> +
> +    NS_WARN_IF_FALSE(canvasElement,
> +        "WebGL HTMLCanvasElement is null. Can't restore");

MOZ_ASSERT. This should never be false.

@@ +203,5 @@
> +
> +    NS_WARN_IF_FALSE(canvasElement,
> +        "WebGL HTMLCanvasElement is null. Can't restore");
> +    if (canvasElement && !canvasElement->OwnerDoc()->Hidden()) {
> +        mContext->ForceRestoreContext();

MOZ_ASSERT(mContext);

@@ +1489,3 @@
>  
>  void
>  WebGLContext::ForceLoseContext()

Add a bool arg for whether or not this should be 'simulated', instead of relying on external state (mLastLossWasSimulated) to be set in a particular order with regards to the call-time of this function.

::: content/canvas/src/WebGLContextGL.cpp
@@ +3832,5 @@
>  {
>      if (IsContextLost())
>          return ErrorInvalidOperation("loseContext: Context is already lost.");
>  
> +    mLastLossWasSimulated = true;

This *must* go after ForceLoseContext, as ForceLoseContext sets this to false.

Comment 46

[Kelsey Gilbert [:jgilbert]]

The "This *must* go after ForceLoseContext, as ForceLoseContext sets this to false." comment was wrong, since you changed that function's behavior. Make this callsite call ForceLoseContext(true).

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #44)
> (In reply to Jerry Shih[:jerry] from comment #43)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #40)
> > I perfer use the warnning message.
> > Do we never get the same situation as [1]?
> > 
> > [1]
> > http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/
> > WebGLContext.cpp#1203
> 
> Warnings are basically useless, since we don't really check them. Asserts is
> the way to be sure we're not messing up. Switch this to an assert, and if
> it's something we should be worried about possibly hitting, give it a safe
> return value in non-DEBUG code.
> 
> Don't accept bad coding practices just because they already made it into the
> tree.
> 
> I'm not entirely sure what you're pointing out here. Is it that we don't
> check the retval of MakeCurrent?

Ohh, the code is change.

I'm talking about this:
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#1137

void
WebGLContext::RobustnessTimerCallback(nsITimer* timer)
{
    if (!mCanvasElement) {
        ....
    }
}

The mCanvasElement maybe is a nullptr. Maybe I will also get the nullptr for canvas element in my code.
If mCanvasElement is nullptr, I think we should show this message for further debugging.
That's why I use NS_WARN_IF_FALSE to show message.

Comment 48

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] from comment #47)
> (In reply to Jeff Gilbert [:jgilbert] from comment #44)
> > (In reply to Jerry Shih[:jerry] from comment #43)
> > > (In reply to Jeff Gilbert [:jgilbert] from comment #40)
> > > I perfer use the warnning message.
> > > Do we never get the same situation as [1]?
> > > 
> > > [1]
> > > http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/
> > > WebGLContext.cpp#1203
> > 
> > Warnings are basically useless, since we don't really check them. Asserts is
> > the way to be sure we're not messing up. Switch this to an assert, and if
> > it's something we should be worried about possibly hitting, give it a safe
> > return value in non-DEBUG code.
> > 
> > Don't accept bad coding practices just because they already made it into the
> > tree.
> > 
> > I'm not entirely sure what you're pointing out here. Is it that we don't
> > check the retval of MakeCurrent?
> 
> Ohh, the code is change.
> 
> I'm talking about this:
> http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/
> WebGLContext.cpp#1137
> 
> void
> WebGLContext::RobustnessTimerCallback(nsITimer* timer)
> {
>     if (!mCanvasElement) {
>         ....
>     }
> }
> 
> The mCanvasElement maybe is a nullptr. Maybe I will also get the nullptr for
> canvas element in my code.
> If mCanvasElement is nullptr, I think we should show this message for
> further debugging.
> That's why I use NS_WARN_IF_FALSE to show message.

Don't use NS_WARN_IF_FALSE to mean 'we should debug if this happens'. Use MOZ_ASSERT. If you just warn, people ignore it. Try it with MOZ_ASSERT, and let's talk if it crashes, because we need to understand why this would fail.

Comment 49

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #48)
> Don't use NS_WARN_IF_FALSE to mean 'we should debug if this happens'. Use
> MOZ_ASSERT. If you just warn, people ignore it. Try it with MOZ_ASSERT, and
> let's talk if it crashes, because we need to understand why this would fail.

Thanks, Jeff.
I will update my patch later.

Comment 50

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #45)
> Comment on attachment 8427624 [details] [diff] [review]
> restore webgl context when visible. v6
> 
> Review of attachment 8427624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, almost there.
> Don't merely WARN and then rely on the warning as a precondition. Use
> MOZ_ASSERT for this. If the assert has any reasonable chance of being hit
> (well, skipped) on non-debug builds, make sure we fail as gracefully as we
> can. (Sometimes MOZ_CRASH is as graceful as is safe)
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +74,5 @@
> >  
> > +WebGLObserver::WebGLObserver(WebGLContext* aContext)
> > +    : mContext(aContext)
> > +{
> > +
> 
> Don't leave empty lines between the brackets.
> 
checked

> @@ +159,5 @@
> > +    nsCOMPtr<nsIObserverService> observerService =
> > +        mozilla::services::GetObserverService();
> > +
> > +    NS_WARN_IF_FALSE(observerService,
> > +        "WebGL ObserverService is null. Can't unregister memory-pressure event");
> 
> When could this be true, such that we shouldn't MOZ_ASSERT it? Mention why
> we can't ASSERT this in a comment.
> 
use MOZ_ASSERT.
checked

> Align the second arg with the first arg. If the line is too long, wrap it
> like:
> Foo(bar,
>     "baz"
>     " bat."); <- this line can have an extra indent if it needs one for
> readability.
> 
> @@ +175,4 @@
> >          return NS_OK;
> > +    }
> > +
> > +    bool wantToLoseContext = mContext->mLoseContextOnMemoryPressure;
> 
> MOZ_ASSERT(mContext);
> 
checked

> @@ +201,5 @@
> > +
> > +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> > +
> > +    NS_WARN_IF_FALSE(canvasElement,
> > +        "WebGL HTMLCanvasElement is null. Can't restore");
> 
> MOZ_ASSERT. This should never be false.
> 
checked

> @@ +203,5 @@
> > +
> > +    NS_WARN_IF_FALSE(canvasElement,
> > +        "WebGL HTMLCanvasElement is null. Can't restore");
> > +    if (canvasElement && !canvasElement->OwnerDoc()->Hidden()) {
> > +        mContext->ForceRestoreContext();
> 
> MOZ_ASSERT(mContext);
> 
checked

> @@ +1489,3 @@
> >  
> >  void
> >  WebGLContext::ForceLoseContext()
> 
> Add a bool arg for whether or not this should be 'simulated', instead of
> relying on external state (mLastLossWasSimulated) to be set in a particular
> order with regards to the call-time of this function.
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +3832,5 @@
> >  {
> >      if (IsContextLost())
> >          return ErrorInvalidOperation("loseContext: Context is already lost.");
> >  
> > +    mLastLossWasSimulated = true;
> 
> This *must* go after ForceLoseContext, as ForceLoseContext sets this to
> false.
The ForceLoseContext() now needs additional boolean value.
checked

Comment 51

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: restore webgl context when visible. v7

update for comment 50

Comment 52

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8441272 [details] [diff] [review]
restore webgl context when visible. v7

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

More NS_WARN stuff.
Also, try to keep mContext as const, if you can. (Add a bool mIsDestroyed, instead of implicitly checking for destruction with !mContext)

::: content/canvas/src/WebGLContext.cpp
@@ +70,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::gfx;
>  using namespace mozilla::gl;
>  using namespace mozilla::layers;
> + 

Extra whitespace.

@@ +81,5 @@
> +{
> +}
> +
> +void
> +WebGLObserver::UnregisterObserver()

Call this 'Destroy'.
I also recommend making mContext const, and having a non-const `bool mIsDestroyed`.

@@ +98,5 @@
> +
> +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> +
> +    NS_WARN_IF_FALSE(canvasElement,
> +        "WebGL HTMLCanvasElement is null. Can't register visibilitychange event");

I don't think this can ever be null, can it?

Basically, I need to be convinced to ever see NS_WARN_IF_FALSE.
Things should generally either:
* Never ever happen, but are really dangerous if they do happen. (MOZ_CRASH)
* Never happen, and indicate an error. (MOZ_ASSERT)
* Happen for good/valid reasons. (handle them without spewing a warning)

There is a non-zero set of conditions where NS_WARN is useful, but I need to be convinced (and have a comment) for why we shouldn't either MOZ_ASSERT or instead just handle the failure without warning.

The biggest place for NS_WARN is when the condition is bad, except for during destruction (like browser teardown), or some other exceptional event, like actual context-loss.

@@ +202,5 @@
> +    }
> +
> +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> +
> +    // We should get the HTMLCanvasElement here to check the hidden attribute.

Put this comment before the line where we get the HTMLCanvasElement. Don't explain what we should have done after we've already done it.

@@ +206,5 @@
> +    // We should get the HTMLCanvasElement here to check the hidden attribute.
> +    MOZ_ASSERT(canvasElement);
> +
> +    if (canvasElement && !canvasElement->OwnerDoc()->Hidden()) {
> +        MOZ_ASSERT(mContext);

Don't assert this here, since we will have already segfaulted if mContext was null above.

::: content/canvas/src/WebGLContext.h
@@ +1163,5 @@
>                               GLenum format,
>                               GLenum type,
>                               const GLvoid *data);
>  
> +    void ForceLoseContext(bool aSimulateLosing = false);

Don't use 'a' arg prefixes in WebGL files.

Comment 53

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #52)
> Comment on attachment 8441272 [details] [diff] [review]
> restore webgl context when visible. v7
> 
> Review of attachment 8441272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> More NS_WARN stuff.
Sorry, I don't check all NS_WARN part.

> Also, try to keep mContext as const, if you can. (Add a bool mIsDestroyed,
> instead of implicitly checking for destruction with !mContext)
> 
Sorry, I don't get your idea.
In WebGLObserver::Destroy(), I don't check the "!mContext".
But I check "!mContext" at other function. Is it better to have an additional variable to check the context status?

> ::: content/canvas/src/WebGLContext.cpp
> @@ +70,5 @@
> >  using namespace mozilla::dom;
> >  using namespace mozilla::gfx;
> >  using namespace mozilla::gl;
> >  using namespace mozilla::layers;
> > + 
> 
> Extra whitespace.
> 
checked

> @@ +81,5 @@
> > +{
> > +}
> > +
> > +void
> > +WebGLObserver::UnregisterObserver()
> 
> Call this 'Destroy'.
> I also recommend making mContext const, and having a non-const `bool
> mIsDestroyed`.
> 
checked
But we need to call WebGLContext::ForceLoseContext() in WebGLObserver, we can't use const mContext ptr here.

> @@ +98,5 @@
> > +
> > +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> > +
> > +    NS_WARN_IF_FALSE(canvasElement,
> > +        "WebGL HTMLCanvasElement is null. Can't register visibilitychange event");
> 
> I don't think this can ever be null, can it?
> 
I don't know. But we can see that we check the HTMLCanvasElement at:
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#377
I change to use MOZ_ASSERT() here.

> Basically, I need to be convinced to ever see NS_WARN_IF_FALSE.
> Things should generally either:
> * Never ever happen, but are really dangerous if they do happen. (MOZ_CRASH)
> * Never happen, and indicate an error. (MOZ_ASSERT)
> * Happen for good/valid reasons. (handle them without spewing a warning)
> 
> There is a non-zero set of conditions where NS_WARN is useful, but I need to
> be convinced (and have a comment) for why we shouldn't either MOZ_ASSERT or
> instead just handle the failure without warning.
> 
> The biggest place for NS_WARN is when the condition is bad, except for
> during destruction (like browser teardown), or some other exceptional event,
> like actual context-loss.
> 
> @@ +202,5 @@
> > +    }
> > +
> > +    HTMLCanvasElement* canvasElement = mContext->GetCanvas();
> > +
> > +    // We should get the HTMLCanvasElement here to check the hidden attribute.
> 
> Put this comment before the line where we get the HTMLCanvasElement. Don't
> explain what we should have done after we've already done it.
> 
I just remove these comment becuase these codes are very simple.

> @@ +206,5 @@
> > +    // We should get the HTMLCanvasElement here to check the hidden attribute.
> > +    MOZ_ASSERT(canvasElement);
> > +
> > +    if (canvasElement && !canvasElement->OwnerDoc()->Hidden()) {
> > +        MOZ_ASSERT(mContext);
> 
> Don't assert this here, since we will have already segfaulted if mContext
> was null above.
I check the mContext, and return early if mContext is null.

> 
> ::: content/canvas/src/WebGLContext.h
> @@ +1163,5 @@
> >                               GLenum format,
> >                               GLenum type,
> >                               const GLvoid *data);
> >  
> > +    void ForceLoseContext(bool aSimulateLosing = false);
> 
> Don't use 'a' arg prefixes in WebGL files.
checked

Thanks, Jeff.

Comment 54

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: restore webgl context when visible. v8

update for comment 53

-----
1. I still don't use const mContext in WebGLObserver.
2. I remove all NS_WARN_IF_FALSE().
3. check mContext before using in WebGLObserver.

Comment 55

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8441919 [details] [diff] [review]
restore webgl context when visible. v8

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

Great, thanks.

Comment 56

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try link:
https://tbpl.mozilla.org/?tree=Try&rev=746575f20374

Comment 57

[Jerry Shih[:jerry] (UTC+8) (inactive)]

We got assert at checking canvasElement during CC.

call stack:
WebGLObserver::UnregisterVisibilityChangeEvent() [content/canvas/src/WebGLContext.cpp:122] // the canvas element is null in this situation，we got assere here.
WebGLObserver::Destroy() [content/canvas/src/WebGLContext.cpp:89]
WebGLContext::~WebGLContext() [content/canvas/src/WebGLContext.cpp:322]
WebGL1Context::~WebGL1Context() [obj-firefox/dist/include/mozilla/mozalloc.h:225]
WebGLContext::DeleteCycleCollectable() [content/canvas/src/WebGLContext.cpp:1526]
WebGLContext::cycleCollection::DeleteCycleCollectable(void*) [content/canvas/src/WebGLContext.h:169]

I think we should remove the assert checking for canvasElement.
Ex:
> +    // We should get the HTMLCanvasElement here to check the hidden attribute.
> +    MOZ_ASSERT(canvasElement);    <= remove this
> +
> +    if (canvasElement && !canvasElement->OwnerDoc()->Hidden()) {
> +        MOZ_ASSERT(mContext);

Comment 58

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] from comment #57)
> We got assert at checking canvasElement during CC.
> 
> call stack:
> WebGLObserver::UnregisterVisibilityChangeEvent()
> [content/canvas/src/WebGLContext.cpp:122] // the canvas element is null in
> this situation，we got assere here.
> WebGLObserver::Destroy() [content/canvas/src/WebGLContext.cpp:89]
> WebGLContext::~WebGLContext() [content/canvas/src/WebGLContext.cpp:322]
> WebGL1Context::~WebGL1Context()
> [obj-firefox/dist/include/mozilla/mozalloc.h:225]
> WebGLContext::DeleteCycleCollectable()
> [content/canvas/src/WebGLContext.cpp:1526]
> WebGLContext::cycleCollection::DeleteCycleCollectable(void*)
> [content/canvas/src/WebGLContext.h:169]
> 
> I think we should remove the assert checking for canvasElement.
> Ex:
> > +    // We should get the HTMLCanvasElement here to check the hidden attribute.
> > +    MOZ_ASSERT(canvasElement);    <= remove this
> > +
> > +    if (canvasElement && !canvasElement->OwnerDoc()->Hidden()) {
> > +        MOZ_ASSERT(mContext);

Ok, but only remove it from the Unregister[...] funcs.

Comment 59

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: restore webgl context when visible. v9. r=jgilbert

update for comment 58

Comment 60

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: restore webgl context when visible. v10. r=jgilbert,smaug

rebase patch

Comment 61

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 62

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I got try failed for valgrind opt build at comment 61.
But I pass the test after rebase.
try:
https://tbpl.mozilla.org/?tree=Try&rev=29e62ee9a07f

Comment 63

[Jerry Shih[:jerry] (UTC+8) (inactive)]

please land the attachment 8444287 [details] [diff] [review] to mozilla-central

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f90e81750943

Comment 65

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f90e81750943

Comment 66

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: restore webgl context when visible. (for b2g-2.0)

rebase for b2g-2.0.

Comment 67

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: fix wrong preference name.

memory-preasure => memory-pressure

Comment 68

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8520418 [details] [diff] [review]
restore webgl context when visible. (for b2g-2.0)

1) rebase to 2.0
2) fix the typo for "memory-pressure"

Comment 69

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8520422 [details] [diff] [review]
fix wrong preference name.

memory-preasure => memory-pressure

Comment 70

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8520418 [details] [diff] [review]
restore webgl context when visible. (for b2g-2.0)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 1092975

User impact if declined:
Webgl context will always loss when we turn off the screen. Some webgl game will not show correctly when we turn on the screen.

Testing completed:
Test with the "Skate Jumper" and "Fly with my car" in Bug 1092975 comment 0. The game works well.

Risk to taking this patch (and alternatives if risky):
Small.

String or UUID changes made by this patch:
none

Comment 71

[bhavana bajaj [:bajaj]]

Blocking as this is needed by another 2.0 blocker: 1092975

Comment 72

[bhavana bajaj [:bajaj]]

Comment on attachment 8520418 [details] [diff] [review]
restore webgl context when visible. (for b2g-2.0)

Requesting QA verification of https://bugzilla.mozilla.org/show_bug.cgi?id=1092975 once this lands on 2.0 branch.

Comment 73

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land the attachment 8520418 [details] [diff] [review] to b2g-2.0 branch and attachment 8520422 [details] [diff] [review] to mozilla-central

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/912526b5ac38
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f6be6c9f0051
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/1b1134490959

I landed the typo fix on b2g34. Seemed like the right thing to do.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/912526b5ac38

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/1b1134490959

Comment 78

[Ryan VanderMeulen [:RyanVM]]

This appears to be causing frequent crashes on b2g32 with the following stack and assert:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=61411&repo=mozilla-b2g32_v2_0

Backed out.
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3e842983c83c

Comment 79

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Bug 1029504 has a patch to remove the assert.
https://hg.mozilla.org/integration/mozilla-inbound/rev/de2f8eca84b1

The XPCOM services might no longer available at that time.
Should I request the uplift approval again for bug 1029504?

Comment 80

[Ryan VanderMeulen [:RyanVM]]

Everything needs approval to land, yes.

Comment 81

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Ryan,
We got approval for Bug 1029504.
https://bugzilla.mozilla.org/show_bug.cgi?id=1029504#c9

Please land the attachment 8520418 [details] [diff] [review] to b2g-2.0 branch first, and land the Bug 1029504 after this patch.

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ff8b56669ee4