Gary [:streetwolf52] Reporter
	Description • 9 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.

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

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

	Gary [:streetwolf52] Reporter
	Comment 2 • 9 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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 4 • 9 years ago

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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 5 • 9 years ago

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?

	Timothy Nikkel (:tnikkel)
	Comment 6 • 9 years ago

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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 7 • 9 years ago

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

	Gary [:streetwolf52] Reporter
	Comment 9 • 9 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.

	Jean-Yves Avenard [:jya]
	Comment 11 • 9 years ago

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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 12 • 9 years ago

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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 13 • 9 years ago

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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 14 • 9 years ago

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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 15 • 9 years ago

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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 16 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 18 • 9 years ago

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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 19 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

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

	Gary [:streetwolf52] Reporter
	Comment 21 • 9 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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 22 • 9 years ago

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)

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 23 • 9 years ago

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

	Neil Deakin
	Comment 24 • 9 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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 25 • 9 years ago

We are good to go!

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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 26 • 9 years ago

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

	Gary [:streetwolf52] Reporter
	Comment 27 • 9 years ago

Seems to be working fine now, thanks.

	Carsten Book [:Tomcat]
	Comment 28 • 9 years ago

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