Last Comment Bug 711333 - Race condition when moving and updating textures
: Race condition when moving and updating textures
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P4 normal (vote)
: Firefox 12
Assigned To: Patrick Walton (:pcwalton)
: Sebastian Kaspari (:sebastian)
Depends on:
Blocks: 709492
  Show dependency treegraph
Reported: 2011-12-15 22:39 PST by Patrick Walton (:pcwalton)
Modified: 2012-01-19 09:15 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed patch. (4.64 KB, patch)
2011-12-16 23:34 PST, Patrick Walton (:pcwalton)
bugmail: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Patrick Walton (:pcwalton) 2011-12-15 22:39:01 PST
It's possible for the following chain of events to occur:

(1) GeckoSoftwareLayerClient begins a transaction on the tile layer in beginDrawing().
(2) GSLC acquires the monitor on the layer controller in updateViewport(), which is called by endDrawing().
(3) GSLC moves the tile and the viewport around.
(4) GSLC releases the monitor.
(5) The GL rendering thread acquires the monitor on the layer controller.
(6) In the process of rendering a frame, the rendering thread tries to begin a transaction but fails and continues with the old image in the new position. Now the display is corrupt for one frame.
(7) GSLC ends the transaction.

This usually shows up as one of the flashes when navigating from page to page. It corrects itself after a frame or two but is quite unsightly. The solution should be for GSLC to always hold the monitor around the transaction. Will post a patch to this effect tomorrow.
Comment 1 Patrick Walton (:pcwalton) 2011-12-16 23:34:04 PST
Created attachment 582497 [details] [diff] [review]
Proposed patch.

Proposed patch attached.
Comment 2 Kartikaya Gupta ( 2011-12-17 06:38:31 PST
Comment on attachment 582497 [details] [diff] [review]
Proposed patch.

>diff --git a/widget/src/android/AndroidGraphicBuffer.cpp b/widget/src/android/AndroidGraphicBuffer.cpp
>   bool EnsureInitialized()
>   {
>     if (mInitialized) {
>       return true;
>     }
>+    return false;
>     void *handle = dlopen(ANDROID_EGL_PATH, RTLD_LAZY);

Testing code? r+ assuming this hunk was included accidentally.
Comment 3 Patrick Walton (:pcwalton) 2011-12-17 10:22:37 PST
(In reply to Kartikaya Gupta (:kats) from comment #2)
> Testing code? r+ assuming this hunk was included accidentally.

Actually, code needed to make my Droid X not crash (bug 711694), but good catch :)
Comment 4 Patrick Walton (:pcwalton) 2011-12-30 14:09:32 PST
Comment 5 Phil Ringnalda (:philor) 2011-12-31 19:36:52 PST
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-01-19 08:32:44 PST
Comment on attachment 582497 [details] [diff] [review]
Proposed patch.

[Triage Comment]
just moves synchronization further up the stack. Also this has baked on m-c for 20 days
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-01-19 09:15:22 PST

Note You need to log in before you can comment on or make changes to this bug.