[e10s] Page openings aren't always smooth and often produce a flashing effect.

RESOLVED FIXED in Firefox 42

Status

()

defect
--
major
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: streetwolf, Assigned: gw280)

Tracking

(Blocks 1 bug, {regression, reproducible})

40 Branch
Firefox 42
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm7+, firefox42 fixed)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

4 years ago
The best way to see this is by comparing non-e10s Fx with e10s Fx.  Do as follows:

1. Place Fx into non-e10s mode.
2. Make sure 'nglayout.initialpaint.delay' is set to 250
3. Go to https://sites.google.com/site/sonthakit/bookmarkfaviconchanger

Notice the way the page appears.

1. Place Fx in e10s mode.
2. Go to the url above.

Notice that the yellow background appears before the page is loaded.
It's the same when you also force reload the page without using cache with CTRL+SHIFT+R.
Blocks: e10s
Severity: normal → major
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Ever confirmed: true

Updated

4 years ago
Blocks: e10s-perf
No longer blocks: e10s
Flags: needinfo?(gwright)
Reporter

Comment 2

4 years ago
I just want to clarify that the URL I gave is just one example of the problem.  Sites that have a colored background make the problem more obvious.  Another site is www.neowin.net where you see the blue background before the rest of the page loads.

Setting 'nglayout.initialpaint.delay' to zero mitigates the problem in many cases.
Duplicate of this bug: 1140414
So here, setting 'nglayout.initialpaint.delay' to 0 simply reduces the time the background spends on screen, and on my box at least, it causes an even more obnoxious fast flashing effect. It doesn't mitigate the problem entirely.
Flags: needinfo?(gwright)
I'm flagging some layout folks here who may have a better idea of what's going on. I think it's something to do with paint suppression?
Here is what I think is happening (I haven't verified).

e10s
-page load starts
-presentation created (presshell, frames, etc), painting suppression starts, we start drawing the new document. We draw the background color only (yellow) because painting is suppressed.
-paint suppression ends, we draw the normal page content

non-e10s
-page load starts
--presentation created (presshell, frames, etc), painting suppression starts. When we paint the browser window now nsSubDocumentFrame::GetSubdocumentPresShellForPainting decides to use the old page to draw (both pages are available at this point) since the new page is paint suppressed (not much point in drawing a background only if we have a non-paint suppressed document available).
-paint suppression ends, we now draw the new document.

So in the non-e10s case we completely skip the drawing of only the background of the new page when it is paint suppressed.

I created the non-e10s behaviour many years ago to solve a different problem that it's impossible for us to have anymore. It also seemed logical. But it could probably use tweaking.

So we could just emulate the non-e10s behavior for e10s, or come up with something superior. I'm not sure where/how we decide which document to paint and when to transition between them in the e10s situation.
Kicking this back into triage. This bug was marked tracking+, but the bug I just duped off this was m7.
Assignee: nobody → gwright
Duplicate of this bug: 1165947
Reporter

Comment 9

4 years ago
This problem is one of the major reasons I avoid using e10s.  Having to see sites background flash for a second or two when I first go to them is extremely annoying.  Hope you can get a patch out asap.  Thanks to everyone working to make Fx even better.
Duplicate of this bug: 1170519
Alter reporting 1170519, i player a bit with other browsers to see their behaviours: chrome too has that flashing effect, but it's much shorter than FF with e10s enabled
So basically this stores the previously attached widget listener (an nsView) on the nsBaseWidget. This allows us to call upon that for painting when the current view's frame is paint suppressed.

This is I think the least invasive way to solve this issue.
Attachment #8627891 - Flags: review?(tnikkel)
One question I have is whether we should call WindowResize() on both widget listeners that are currently active.
Updated patch on try which keeps track of the lifetime of the nsView properly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84d713fb798e
Attachment #8627891 - Attachment is obsolete: true
Attachment #8627891 - Flags: review?(tnikkel)
Attachment #8629090 - Flags: review?(tnikkel)
Comment on attachment 8629090 [details] [diff] [review]
0001-Bug-1157941-If-the-current-PresShell-is-suppressed-p.patch

>@@ -440,6 +440,12 @@ nsViewManager::ProcessPendingUpdatesPaint(nsIWidget* aWidget)
>+    if (aWidget->GetPreviouslyAttachedWidgetListener() &&
>+        view->IsPrimaryFramePaintSuppressed()) {
>+      return;
>+    }

Should also check that we aren't the previously attached listener.

>@@ -217,7 +217,13 @@ PuppetWidget::Resize(double aWidth,
>+  // call WindowResized() on both the current listener, and possibly
>+  // also the previous one if we're in a state where we're drawing that one
>+  // because the current one is paint suppressed
>   if (!oldBounds.IsEqualEdges(mBounds) && mAttachedWidgetListener) {
>+    if (GetCurrentWidgetListener()) {
>+      GetCurrentWidgetListener()->WindowResized(this, mBounds.width, mBounds.height);
>+    }
>     mAttachedWidgetListener->WindowResized(this, mBounds.width, mBounds.height);
>   }

If we just have mAttachedWidgetListener this will call WindowResized twice on it.

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h

Need IID bump on nsIWidget.
Attachment #8629090 - Flags: review?(tnikkel) → review+
Reporter

Comment 21

4 years ago
Have you guys got a handle on this problem?  If you rally want to see the bug in action set nglayout.initialpaint.delay = 1000 then got to the link below with e10s and non e10s. e10s will show the blue background for about a second, non e10s will show no blue background at all.


http://www.neowin.net/forum/topic/1228511-answer-the-question-and-make-your-own/?view=getnewpost
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d0c3c530c17

So this passes on the trybot loaner I have. Let's see if it's landable!

(thanks to Enn for the tip regarding using BrowserTestUtils to do the tab switching)
Carrying forward r+ from tn as none of that code has changed, but flagging Neil for review to cover the test changes.
Attachment #8629090 - Attachment is obsolete: true
Attachment #8636681 - Flags: review?(enndeakin)

Comment 24

4 years ago
Comment on attachment 8636681 [details] [diff] [review]
cache-previous-listener.patch

>-  gBrowser.selectedTab = gBrowser.addTab(URL);
>-  let browser = gBrowser.selectedBrowser;
>-  yield BrowserTestUtils.browserLoaded(browser);
>+  let testTab = gBrowser.addTab(URL);
>+
>+  yield BrowserTestUtils.switchTab(gBrowser, testTab);
>+

You can also use openNewForegroundTab here.


>+  yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
>+  let blankTab = gBrowser.selectedTab;

openNewForegroundTab yields the new tab.
Attachment #8636681 - Flags: review?(enndeakin) → review+
Reporter

Comment 27

4 years ago
Seems to be working fine now, thanks.
https://hg.mozilla.org/mozilla-central/rev/60f82e40f039
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Updated

4 years ago
Depends on: 1187990
See Also: → 1323586
You need to log in before you can comment on or make changes to this bug.