Closed Bug 772672 Opened 12 years ago Closed 11 years ago

CreateCompositor can hold up the Gecko thread, waiting for a surface

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gbrown, Assigned: kats)

References

Details

Attachments

(1 file)

CreateCompositor waits indefinitely for a surface. If the awesome screen is brought up immediately after startup, CreateCompositor may be called after surfaceDestroyed has executed and before any surfaceCreated/Changed. surfaceChanged will not execute until after the awesome screen is exited. 

To be clear, order of events is:
<start Fennec, click on awesome bar quickly>
surfaceDestroyed
CreateCompositor
waitForValidSurface
<nothing happens on Gecko thread until awesome screen exited>
surfaceChanged
waitForValidSurface ends
CreateCompositor ends

I cannot reproduce any user-visible problem associated with this condition - the awesome screen appears to work fine, and CreateCompositor completes once the awesome screen is exited. However, blocking the Gecko thread for this length of time seems...unexpected and potentially problematic. It causes a Robocop test failure (bug 769524); we can work around that if necessary, but I want to check that this is working as intended. 

(We just resolved a similar issue in bug 771189.)
Blocks: 769524
See Bug 769269, Comment 78, for a couple approaches we could follow to reduce blocking during compositor creation.
Attached patch patchSplinter Review
Assignee: nobody → blassey.bugs
Attachment #641583 - Flags: review?(ajuma)
Comment on attachment 641583 [details] [diff] [review]
patch

This looks good, under the assumption that whatever triggers the surface to become valid again (e.g. leaving the awesomescreen) also triggers a draw event (and hence causes the compositor to get created). If this assumption turns out to be false, we won't paint until something else triggers a draw event. So we should keep an eye out for lack-of-painting-at-startup bugs after this lands; if these occur, we'll need to send a draw event to Gecko whenever the surface becomes valid.
Attachment #641583 - Flags: review?(ajuma) → review+
Unfortunately, this had to be backed out due to robocop orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/71056117a930

https://tbpl.mozilla.org/php/getParsedLog.php?id=13472571&tree=Mozilla-Inbound

3 INFO TEST-UNEXPECTED-FAIL | testLoad | PaintExpecter - blockUtilClear timeout
junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testLoad | PaintExpecter - blockUtilClear timeout
4 INFO TEST-UNEXPECTED-FAIL | testLoad | Exception caught - junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testLoad | PaintExpecter - blockUtilClear timeout
3 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | PaintExpecter - blockUtilClear timeout
junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | PaintExpecter - blockUtilClear timeout
4 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | Exception caught - junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testPanCorrectness | PaintExpecter - blockUtilClear timeout
3 INFO TEST-UNEXPECTED-FAIL | testFlingCorrectness | PaintExpecter - blockUtilClear timeout
junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testFlingCorrectness | PaintExpecter - blockUtilClear timeout
4 INFO TEST-UNEXPECTED-FAIL | testFlingCorrectness | Exception caught - junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testFlingCorrectness | PaintExpecter - blockUtilClear timeout
3 INFO TEST-UNEXPECTED-FAIL | testOverscroll | PaintExpecter - blockUtilClear timeout
junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testOverscroll | PaintExpecter - blockUtilClear timeout
4 INFO TEST-UNEXPECTED-FAIL | testOverscroll | Exception caught - junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testOverscroll | PaintExpecter - blockUtilClear timeout
3 INFO TEST-UNEXPECTED-FAIL | testAxisLocking | PaintExpecter - blockUtilClear timeout
junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testAxisLocking | PaintExpecter - blockUtilClear timeout
4 INFO TEST-UNEXPECTED-FAIL | testAxisLocking | Exception caught - junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testAxisLocking | PaintExpecter - blockUtilClear timeout
4 INFO TEST-UNEXPECTED-FAIL | testWebContentContextMenu | Exception caught - junit.framework.AssertionFailedError: The text: Open is not found!
We might be running into the scenario discussed in Comment 3. To test this theory, I pushed the patch to try with a small change to GeckoLayerClient that ensures we trigger a draw event once the surface becomes valid:

https://tbpl.mozilla.org/?tree=Try&rev=2a491a580474
(In reply to Ali Juma [:ajuma] from comment #5)
> We might be running into the scenario discussed in Comment 3. To test this
> theory, I pushed the patch to try with a small change to GeckoLayerClient
> that ensures we trigger a draw event once the surface becomes valid:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=2a491a580474

Robocop is still orange with this change.
Assignee: blassey.bugs → snorp
If this is indeed what blocks bug 769524, this is now a blocker for ongoing Snappy work.
Severity: normal → blocker
I verified that this is (still) the cause of bug 769524: see https://bugzilla.mozilla.org/show_bug.cgi?id=769524#c88.

I also note that the existing workaround for bug 769524 seems quite effective: no test failures for a long time now.
Unfortunately, if as I understand this is also the problem that prevents bug 760036 from landing, the workaround is not sufficient.
No longer blocks 760036.
Severity: blocker → normal
Depends on: 844275
The behaviour described by the summary (CreateCompositor blocks the Gecko thread for a long time) should no longer happen as of the landing of bug 844275. In the new code, CreateCompositor itself will never be called unless we have a surface available, so it should should not block the Gecko thread indefinitely, although we still won't have a compositor until after the awesome screen is dismissed (using the STR in comment 0). Marking this as fixed.
Assignee: snorp → bugmail.mozilla
Status: NEW → RESOLVED
Closed: 11 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: