Closed
Bug 1132072
Opened 10 years ago
Closed 10 years ago
[e10s] black frame sometimes seen when switching tabs
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 39
People
(Reporter: gw280, Assigned: billm)
References
Details
Attachments
(3 files, 2 obsolete files)
29.08 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
3.51 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
Sometimes on my Linux box I see a black frame being rendered just after switching tabs, and just before the spinner is drawn. This is on a local build of m-c from 2015-02-10 (optimised, profiling). I actually have a profile of it too as I was profiling at the time: http://people.mozilla.org/~gwright/WUqSl3IG.bin
Adapter Description Intel Open Source Technology Center -- Mesa DRI Intel(R) Haswell
Device ID Mesa DRI Intel(R) Haswell
Driver Version 3.0 Mesa 10.4.2
GPU Accelerated Windows 0/3 Basic (OMTC)
Vendor ID Intel Open Source Technology Center
WebGL Renderer Intel Open Source Technology Center -- Mesa DRI Intel(R) Haswell
windowLayerManagerRemote true
AzureCanvasBackend cairo
AzureContentBackend cairo
AzureFallbackCanvasBackend none
AzureSkiaAccelerated 0
Reporter | ||
Updated•10 years ago
|
Blocks: e10s-spinner
tracking-e10s:
--- → ?
Assignee | ||
Comment 1•10 years ago
|
||
This is the patch I've been working on.
Updated•10 years ago
|
Assignee: nobody → wmccloskey
Assignee | ||
Comment 3•10 years ago
|
||
Here's the latest version of the patch. There's still one intermittent I need to track down, but it's basically done.
Attachment #8563454 -
Attachment is obsolete: true
Attachment #8573454 -
Flags: review?(mconley)
Comment 4•10 years ago
|
||
Comment on attachment 8573454 [details] [diff] [review]
Tab switch refactoring
Review of attachment 8573454 [details] [diff] [review]:
-----------------------------------------------------------------
I think I understand what you're doing here, and I'm glad you're centralizing all of the async tab switching stuff into a single object.
So one major problem I seem to have with this patch is that I restored a session with multiple tabs, and my first tab was stuck with the spinner forever. This means that I never called finish, which means that I was hitting onUnloadTimeout forever. This kills the battery. So there's a bug there.
I also have some minor nits / questions. See below.
::: browser/base/content/tabbrowser.xml
@@ +2872,5 @@
> + The tab switcher is a state machine. For each tab, it
> + maintains state about whether the layer tree for the tab is
> + available, being loaded, being unloaded, or unavailable. It
> + also keeps track of the tab currently being displayed, the tab
> + it's trying to load, and the tab the user has asked to switch
Might be worth mentioning the lifetime of the tab switcher too - I thought it existed for the lifetime of a tabbrowser, and was initted and memoized after the first tab switch. That's actually not the case.
@@ +2932,5 @@
> +
> + let switcher = {
> + // How long to wait for a tab's layers to load. After this
> + // time elapses, we're free to put up the spinner and start
> + //trying to load a different tab.
Nit - space before trying
@@ +2971,5 @@
> + // visible. All tabs but the most recently shown tab are
> + // removed from the set upon MozAfterPaint.
> + maybeVisibleTabs: new Set([this.selectedTab]),
> +
> + STATE_UNLOADED: 0,
Some inconsistency here - we've got the kPrefix for the timeout lengths, but ALL_CAPS for the states. Let's pick one and stick with it.
@@ +2976,5 @@
> + STATE_LOADING: 1,
> + STATE_LOADED: 2,
> + STATE_UNLOADING: 3,
> +
> + // Enable logging?
Nit - I don't think this comment adds much, tbh.
@@ +3008,5 @@
> + if (state == this.STATE_LOADING) {
> + // Ask for a MozLayerTreeReady event. If the tab is in
> + // the process of initializing, requestNotifyLayerTreeReady
> + // will throw, so we retry until it works.
> + tryUntilSuccess(fl.requestNotifyLayerTreeReady, 10);
If the content process crashes before we can get the layer tree ready notification, I think this will run forever. Am I right on that? It might be worth having some kind of fallback termination policy here...
Alternatively, is there no way of getting an event on initialization that we can listen for?
The 10 ms magic number should probably get thrown into the constants at the top of the switcher.
@@ +3101,5 @@
> + let tabPanel = this.tabbrowser.mPanelContainer;
> + let showPanel = tabs.getRelatedElement(showTab);
> + let index = Array.indexOf(tabPanel.childNodes, showPanel);
> + if (index != -1) {
> + this.log("Switch to tab " + index + " " + this.tinfo(showTab));
You might like the new backtick powers that ES6 gives us:
this.log(`Switch to tab ${index} ${this.tinfo(showTab)}`);
@@ +3154,5 @@
> + this.loadTimer = null;
> + }
> + },
> +
> + // This code runs after we've responded to an event and updated all
"responded to an event", or had a new tab requested.
@@ +3158,5 @@
> + // This code runs after we've responded to an event and updated all
> + // the principal state variables. It takes care of updating any
> + // auxilliary state.
> + postActions: function() {
> + this.assert(!this.loadingTab ||
Can you document why these assertions are necessary?
@@ +5825,5 @@
> let toTab = this.getRelatedElement(this.childNodes[val]);
> let fromTab = this._selectedPanel ? this.getRelatedElement(this._selectedPanel)
> : null;
>
> + let switcher = gBrowser._getSwitcher();
Might as well just do:
gBrowser._getSwitcher().requestTab(toTab);
Attachment #8573454 -
Flags: review?(mconley) → review-
Assignee | ||
Updated•10 years ago
|
Component: Graphics → General
Product: Core → Firefox
Assignee | ||
Comment 6•10 years ago
|
||
I decided to move the failure handling for requestNotifyLayerTreeReady to the platform side, which cleans things up a bit.
I also turned off the unload timer if we don't have anything to unload. That should mitigate the battery issue, althoug obviously we want to avoid the spinner anyway.
Attachment #8573454 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
David, this patch adds on a patch in bug 129223. The problem is in how we handle the case where the frontend asks for a notification that a tab's layer tree is ready before the PRenderFrame has been constructed. I think the easiest way to handle this is to set a flag that we check when the PRenderFrame eventually arrives.
Comment 8•10 years ago
|
||
Hey billm - was attachment 8575622 [details] [diff] [review] ready for review? Or were you holding off on r? for something?
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8575622 [details] [diff] [review]
Tab switch refactoring (r=mconley?)
I've started using a git tool to post my patches. I must have messed up.
Flags: needinfo?(wmccloskey)
Attachment #8575622 -
Flags: review?(mconley)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8575623 [details] [diff] [review]
Handle RequestNotifyLayerTreeReady when RenderFrameParent not ready
See comment 7.
Attachment #8575623 -
Flags: review?(dvander)
Updated•10 years ago
|
Attachment #8575623 -
Flags: review?(dvander) → review+
Comment 11•10 years ago
|
||
Comment on attachment 8575622 [details] [diff] [review]
Tab switch refactoring (r=mconley?)
Review of attachment 8575622 [details] [diff] [review]:
-----------------------------------------------------------------
I think we should go for this. I like how it's encapsulating all of the complexity of our tab switching behaviour into a single place. I also think this greatly reduces the weird conditions we could get into before with the Promises.
Assuming a green try run, r=me. Thanks billm!
Attachment #8575622 -
Flags: review?(mconley) → review+
Reporter | ||
Comment 12•10 years ago
|
||
Bill, I'm seeing a test timeout failure with your patch on browser/base/content/test/general/browser_tabfocus.js
See https://treeherder.mozilla.org/#/jobs?repo=try&revision=7be6c40dee88 - any of the bc1-e10s are showing timeouts. The other failures were due to my patch, but it appears the timeout at the end of the test is from this one.
Assignee | ||
Comment 13•10 years ago
|
||
Yeah, I'll fix it soon. I've been trying to finish some reviews first.
Reporter | ||
Comment 14•10 years ago
|
||
No problem, just wanted to make sure you were aware of it.
Assignee | ||
Comment 15•10 years ago
|
||
This fixes the test issue for me. The problem is that the tab switcher sometimes switches to the new tab immediately if we've already been waiting a long time for the current tab to load and it still hasn't loaded. After the switcher runs, we call updateCurrentBrowser, which then unfocuses the current tab (which is actually the new tab), thinking that we'll focus it when we switch to it. But that never happens.
The fix is to avoid the blurring behavior if we've already switched. We don't need to call _adjustFocusAfterTabSwitch because the switcher already did that when it switched.
Attachment #8579676 -
Flags: review?(mconley)
Updated•10 years ago
|
Attachment #8579676 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 16•10 years ago
|
||
I had to back these out in
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5cf44338f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a27defc5a71d
https://hg.mozilla.org/integration/mozilla-inbound/rev/84bbf01b30ba
for apparently causing near-permafailures in e10s-browser-chrome-3:
https://treeherder.mozilla.org/logviewer.html#?job_id=7881250&repo=mozilla-inbound
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 19•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/27d2e35e5832
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/564a9e11d9ec
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/27d2e35e5832
https://hg.mozilla.org/mozilla-central/rev/564a9e11d9ec
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
You need to log in
before you can comment on or make changes to this bug.
Description
•