Closed Bug 823610 Opened 7 years ago Closed 7 years ago

Contact image only intermittently loads into crop view of pick activity

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18+ fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 + fixed

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
blocking-basecamp: ? → -
tracking-b2g18: --- → +
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?
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.
blocking-basecamp: - → ?
Summary: Contact image intermittently fails to load into crop view of pick activity → Contact image only intermittently loads into crop view of pick activity
Note, many of those 30 tries have been outside the memory tests, just in "normal" usage.
Triage:BB-, tracking-b2g18+
blocking-basecamp: ? → -
Triage:BB+, P3, C3 - severe usability issue. please ignore comment 4
blocking-basecamp: - → +
Priority: -- → P3
Target Milestone: --- → B2G C4 (2jan on)
Did some testing I think comment 1 is right.  I see a memory-pressure event fired in the gallery right when this happens.
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
In the style of bug 818766.

GL hunks -> jgilbert, dom/ipc -> jlebar.
Assignee: nobody → jones.chris.g
Attachment #696852 - Flags: review?(justin.lebar+bug)
Attachment #696852 - Flags: review?(jgilbert)
Attachment #696852 - Flags: feedback?(bjacob)
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.
Attachment #696852 - Flags: feedback?(bjacob) → feedback+
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.
Attachment #696852 - Flags: review?(jgilbert) → review+
Does not seems a Gaia bug anymore. Let's switch the component to reflect that.
Component: Gaia::Gallery → General
QA Contact: jhammink
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 ...
Attachment #697289 - Flags: review?(justin.lebar+bug)
Attachment #697289 - Flags: review?(jgilbert)
Attachment #697289 - Flags: review?
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.
Attachment #697289 - Flags: review?(jgilbert) → review+
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.  :)
Attachment #697289 - Flags: review?(justin.lebar+bug) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/fc3fd7c51000
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 827217
You need to log in before you can comment on or make changes to this bug.