823610 - Contact image only intermittently loads into crop view of pick activity

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P3)

Description

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

STR
 (1) Follow test at https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW2:_Active_call_stays_active

When loading the crop view, I sometimes see the image not load.  I don't see any errors in logcat, though this test is explicitly designed to create memory pressure so that might be a factor.

bb? to get on radar, not suggesting we block.

Comment 1

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

I'm 99.9% sure now that this is caused by the gallery picker's WebGL context being killed off by a memory-pressure event.

 (1) Do we really want to do this to foreground apps?
 (2) What's the minimal patch to fix the gallery?

Comment 2

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

I'm not so sure about comment 1 anymore.

However, in the last several days, I have seen this work exactly *once* in maybe 30 tries.  I don't believe that people testing this (if any) have been testing in realistic conditions.

This is a broken feature that we should either fix or cut.

Comment 3

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

Note, many of those 30 tries have been outside the memory tests, just in "normal" usage.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

Triage:BB-, tracking-b2g18+

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

Triage:BB+, P3, C3 - severe usability issue. please ignore comment 4

Comment 6

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

Did some testing I think comment 1 is right.  I see a memory-pressure event fired in the gallery right when this happens.

Comment 7

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

Confirmed.

Comment 8

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

That's weird.  I started adding contextlost handling to [1], but it wasn't trivial.  But [2] implied it would be! ;)

It doesn't really make sense to me to fire contextlost on visible canvases.  That's pretty hard to determine in general, but we definitely know that canvases in background windows aren't visible.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/ImageEditor.js#L612
[2] http://www.khronos.org/webgl/wiki/HandlingContextLost

Comment 9

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

Attachment: Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g

In the style of bug 818766.

GL hunks -> jgilbert, dom/ipc -> jlebar.

Comment 10

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

Comment on attachment 696852 [details] [diff] [review]
Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g

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

WebGL and pref parts look good.

Comment 11

[Jeff Gilbert [:jgilbert]]

Comment on attachment 696852 [details] [diff] [review]
Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g

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

Looks fine, though the name for the var could be improved. To that end, the pref should probably be kept matching if the var changes.

::: content/canvas/src/WebGLContext.cpp
@@ +67,5 @@
>      bool wantToLoseContext = true;
>  
> +    if (!mContext->mLoseContextInForeground && IsCurrentProcessForeground())
> +        wantToLoseContext = false;
> +    else if (!nsCRT::strcmp(aSomeData,

I still dislike `!strcmp()` as a meme, but this code was already here, so don't worry about it.

@@ +183,5 @@
>      mContextRestorer = do_CreateInstance("@mozilla.org/timer;1");
>      mContextStatus = ContextStable;
>      mContextLostErrorSet = false;
>      mLoseContextOnHeapMinimize = false;
> +    mLoseContextInForeground = true;

`mCanLoseContextInForeground` is probably better. `mLoseContextInForeground` sounds like a really silly thing to do.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Does not seems a Gaia bug anymore. Let's switch the component to reflect that.

Comment 13

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

Attachment: Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g , v1.1

Updated names.

Comment 14

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

Comment on attachment 697289 [details] [diff] [review]
Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g , v1.1

Let's try again ...

Comment 15

[Jeff Gilbert [:jgilbert]]

Comment on attachment 697289 [details] [diff] [review]
Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g , v1.1

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

Cool, thanks.

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 697289 [details] [diff] [review]
Make discarding GL contexts of foreground pages on memory pressure pref-able, and pref off for b2g , v1.1

>diff --git a/content/canvas/src/WebGLContextValidate.cpp b/content/canvas/src/WebGLContextValidate.cpp
>index be6c13e..5052460 100644
>--- a/content/canvas/src/WebGLContextValidate.cpp
>+++ b/content/canvas/src/WebGLContextValidate.cpp

>@@ -700,16 +700,17 @@ WebGLContext::InitAndValidateGL()
>     if (error != LOCAL_GL_NO_ERROR) {
>         GenerateWarning("GL error 0x%x occurred during OpenGL context initialization, before WebGL initialization!", error);
>         return false;
>     }
> 
>     mMinCapability = Preferences::GetBool("webgl.min_capability_mode", false);
>     mDisableExtensions = Preferences::GetBool("webgl.disable-extensions", false);
>     mLoseContextOnHeapMinimize = Preferences::GetBool("webgl.lose-context-on-heap-minimize", false);
>+    mCanLoseContextInForeground = Preferences::GetBool("webgl.can-lose-context-in-foreground", true);

The top of all.js says

> * SYNTAX HINTS:
> *
> *  - Dashes are delimiters; use underscores instead.

Is that wrong?  (Sure seems like it.)

>diff --git a/dom/ipc/ProcessPriorityManager.h b/dom/ipc/ProcessPriorityManager.h
>index c320611..f149a30 100644
>--- a/dom/ipc/ProcessPriorityManager.h
>+++ b/dom/ipc/ProcessPriorityManager.h

>@@ -21,13 +21,20 @@ namespace ipc {
>  *
>  * Hal may adjust this process's operating system priority (e.g. niceness, on
>  * *nix) according to these notificaitons.
>  *
>  * This function call does nothing if the pref for OOP tabs is not set.
>  */
> void InitProcessPriorityManager();
> 
>+/**
>+ * True iff the current process has foreground or higher priority as
>+ * computed by DOM visibility.  The returned answer may not match the
>+ * actual OS process priority, for short intervals.
>+ */
>+bool IsCurrentProcessForeground();

Would you mind "CurrentProcessIsForeground"?

>diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
>index 749dea5..e8a66b8 100644
>--- a/dom/ipc/ProcessPriorityManager.cpp
>+++ b/dom/ipc/ProcessPriorityManager.cpp

>@@ -378,15 +384,22 @@ InitProcessPriorityManager()
>   // backgrounded).
>   if (XRE_GetProcessType() == GeckoProcessType_Default) {
>     LOG("This is the master process.");
>     hal::SetProcessPriority(getpid(), PROCESS_PRIORITY_MASTER);
>     return;
>   }
> 
>   // This object is held alive by the observer service.
>-  nsRefPtr<ProcessPriorityManager> mgr = new ProcessPriorityManager();
>-  mgr->Init();
>+  sManager = new ProcessPriorityManager();
>+  sManager->Init();
>+  ClearOnShutdown(&sManager);

This comment is no longer right.

>+bool
>+IsCurrentProcessForeground()
>+{
>+  return sManager->GetPriority() >= PROCESS_PRIORITY_FOREGROUND;
> }

Can you add a comment on the enum definition that anything greater than or
equal to FOREGROUND is considered "foreground"?

Otherwise I don't mind explicitly listing the possibilities here, since there
are only two.  :)

Comment 17

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

(In reply to Justin Lebar [:jlebar] from comment #16)
> Comment on attachment 697289 [details] [diff] [review]
> The top of all.js says
> 
> > * SYNTAX HINTS:
> > *
> > *  - Dashes are delimiters; use underscores instead.
> 
> Is that wrong?  (Sure seems like it.)

No clue.  I'm just blindly following existing webgl pref style.

> 
> >diff --git a/dom/ipc/ProcessPriorityManager.h b/dom/ipc/ProcessPriorityManager.h
> >index c320611..f149a30 100644
> >--- a/dom/ipc/ProcessPriorityManager.h
> >+++ b/dom/ipc/ProcessPriorityManager.h
> 
> >@@ -21,13 +21,20 @@ namespace ipc {
> >  *
> >  * Hal may adjust this process's operating system priority (e.g. niceness, on
> >  * *nix) according to these notificaitons.
> >  *
> >  * This function call does nothing if the pref for OOP tabs is not set.
> >  */
> > void InitProcessPriorityManager();
> > 
> >+/**
> >+ * True iff the current process has foreground or higher priority as
> >+ * computed by DOM visibility.  The returned answer may not match the
> >+ * actual OS process priority, for short intervals.
> >+ */
> >+bool IsCurrentProcessForeground();
> 
> Would you mind "CurrentProcessIsForeground"?
> 

Fine by me.

> >diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
> >index 749dea5..e8a66b8 100644
> >--- a/dom/ipc/ProcessPriorityManager.cpp
> >+++ b/dom/ipc/ProcessPriorityManager.cpp
> 
> >@@ -378,15 +384,22 @@ InitProcessPriorityManager()
> >   // backgrounded).
> >   if (XRE_GetProcessType() == GeckoProcessType_Default) {
> >     LOG("This is the master process.");
> >     hal::SetProcessPriority(getpid(), PROCESS_PRIORITY_MASTER);
> >     return;
> >   }
> > 
> >   // This object is held alive by the observer service.
> >-  nsRefPtr<ProcessPriorityManager> mgr = new ProcessPriorityManager();
> >-  mgr->Init();
> >+  sManager = new ProcessPriorityManager();
> >+  sManager->Init();
> >+  ClearOnShutdown(&sManager);
> 
> This comment is no longer right.
> 

Removed.

> >+bool
> >+IsCurrentProcessForeground()
> >+{
> >+  return sManager->GetPriority() >= PROCESS_PRIORITY_FOREGROUND;
> > }
> 
> Can you add a comment on the enum definition that anything greater than or
> equal to FOREGROUND is considered "foreground"?
> 

Done.

Comment 18

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

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

Comment 19

[Ed Morley [:emorley]]

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

Comment 20

[Kan-Ru Chen [:kanru] (UTC+8)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1aae9f7f0cf5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0a21cf9f0047