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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Gary [:streetwolf52]]

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.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

It's the same when you also force reload the page without using cache with CTRL+SHIFT+R.

Comment 2

[Gary [:streetwolf52]]

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.

Comment 4

[George Wright (:gw280) (needinfo me!)]

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.

Comment 5

[George Wright (:gw280) (needinfo me!)]

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?

Comment 6

[Timothy Nikkel (:tnikkel)]

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.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Kicking this back into triage. This bug was marked tracking+, but the bug I just duped off this was m7.

Comment 9

[Gary [:streetwolf52]]

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.

Comment 11

[Jean-Yves Avenard [:jya]]

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

Comment 12

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1157941-If-the-current-PresShell-is-suppressed-p.patch

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.

Comment 13

[George Wright (:gw280) (needinfo me!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54e0721274de

Comment 14

[George Wright (:gw280) (needinfo me!)]

One question I have is whether we should call WindowResize() on both widget listeners that are currently active.

Comment 15

[George Wright (:gw280) (needinfo me!)]

Updated patch on try which keeps track of the lifetime of the nsView properly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84d713fb798e

Comment 16

[George Wright (:gw280) (needinfo me!)]

Found a bug, updated try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a7af27b3430

Comment 17

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1157941-If-the-current-PresShell-is-suppressed-p.patch

Comment 18

[Timothy Nikkel (: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.

Comment 19

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57f4bd1baf13

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX 10.10 e10s browser_bug427559.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=11423680&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/a026b731506a

Comment 21

[Gary [:streetwolf52]]

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

Comment 22

[George Wright (:gw280) (needinfo me!)]

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)

Comment 23

[George Wright (:gw280) (needinfo me!)]

Attachment: cache-previous-listener.patch

Carrying forward r+ from tn as none of that code has changed, but flagging Neil for review to cover the test changes.

Comment 24

[Neil Deakin]

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.

Comment 25

[George Wright (:gw280) (needinfo me!)]

We are good to go!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e190da4b85fb

Comment 26

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60f82e40f039

Comment 27

[Gary [:streetwolf52]]

Seems to be working fine now, thanks.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60f82e40f039