Jesse Ruderman Reporter
	Description • 10 years ago

1. Minimize an e10s browser window (⌘M or click the yellow thing)
2. Restore it (click the dock icon)

Result: the entire content area flashes white just after the window is restored to full-size, then the content repaints

[Tested using Nightly 36.0a1 (2014-11-12) on Mac OS X 10.9.5, with the dock minimize animation set to "scale effect".]

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

I'm hoping this is a dupe of bug 1157941

	Markus Stange [:mstange]
	Comment 4 • 9 years ago

Using paint flashing, I see two invalidations between the before-minimize and after-unminimize states. But I don't see any white flashing.

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

So it's coming from here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#10660

The question is, how do we deal with this? It's by design, but obviously this behaviour is undesirable.

Would adding paint suppression here be the right thing to do? The idea being that hopefully if we suppress painting for long enough, the background won't get painted over the old content and cause the flashing.

	Markus Stange [:mstange]
	Comment 6 • 9 years ago

> // PresShell::SetIsActive() is the first C++ entry point at which we
> // (i) know that our parent process wants our content to be hidden

I think this assumption is slightly incorrect. PresShell::SetIsActive(false) is called both for background tabs in a window, and for foreground tabs in minimized windows. We only want to hide the content in the former case, I think.

Not sure how to fix this. Maybe we need a new method like PresShell::SetIsHidden that gets called in the same places as PresShell::SetIsActive except for the minimize window case?

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

This adds a new attribute "setIsActiveAndForeground" to say that we're changing the active state of a docshell, but that it's still foreground and so don't throw our layers. I think this is the least invasive way of doing it, as tracking down all the callsites of SetIsActive is going to be quite messy.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 9 years ago

Comment on attachment 8681673 [details] [diff] [review]
0001-Bug-1098131-Don-t-invalidate-layers-when-simply-chan.patch

>-nsDocShell::SetIsActive(bool aIsActive)
>+nsDocShell::SetIsActiveAndForeground(bool aIsActive) {
{ goes to its own line



>+nsDocShell::SetIsActive(bool aIsActive) {
ditto




>+nsDocShell::SetIsActiveInternal(bool aIsActive, bool aIsHidden)
You need to deal with child docshells too here, and pass aIsHidden to them
So, in this method there is SetIsActive call, that should become SetIsActiveInternal

> 
>+  virtual nsresult SetIsActiveInternal(bool aIsActive, bool aIsHidden);
I don't see any reason for 'virtual'

>+++ b/docshell/base/nsIDocShell.idl
>@@ -625,6 +625,12 @@ interface nsIDocShell : nsIDocShellTreeItem
update uuid when changing idl interfaces

>+PresShell::SetIsActive(bool aIsActive, bool aIsHidden)
Could you still verify that we get another call here with aIsHidden true if a tab is opened when browser is minimized.
Assuming that is true, r+ (+ comments fixed.)

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

OK, I've confirmed that aIsHidden is true if a tab is opened when the browser is minimised. Going to land this with your changes as requested.

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

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

	Pulsebot
	Comment 11 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41523aaefeaf

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 12 • 9 years ago

Backed out by https://hg.mozilla.org/integration/mozilla-inbound/rev/41523aaefeaf because of failures like

Linux x64 pgo M(1):
test_cache_orphaned_body.html | disk usage should have grown

Windows 8 x64 debug M(JP):
/test-test-memory.js.testGC | the weakref returned undefined - { // [object Object]

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

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

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

So the test failure was due to the way we grab the active state of the docshell in e10s. We basically cache the state on the TabParent, and ensure that all setting/requesting goes via the TabParent. My original patch kind of short circuited that by setting isActive on the docshell directly, so the TabParent's cache never got updated.

I'm not hugely happy about adding this weirdly-named "docShellIsActiveAndForeground" attribute to nsITabParent as well as the nsIDocShell, but I can't really think of a cleaner way of doing this without changing the attribute into a method with an aIsHidden parameter, and changing all the callsites. I have ensured I have put lots of documentation in to make it clear what the attribute does, though.

As the patch has changed a fair bit since the previous incarnation, I'm flagging it for review again. Jim, as you worked on bug 1199765 I'd like to hear your feedback as well.

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

Full try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be88d355ee36

	Jim Mathies [:jimm]
	Comment 16 • 9 years ago

Comment on attachment 8685237 [details] [diff] [review]
0001-Bug-1098131-Don-t-invalidate-layers-when-simply-chan.patch

Review of attachment 8685237 [details] [diff] [review]:
-----------------------------------------------------------------

smaug's feedback is valuable here too, he might have a different preference on this.

::: docshell/base/nsIDocShell.idl
@@ +631,5 @@
>    /**
> +   * Sets whether a docshell is active, as above, but ensuring it does
> +   * not discard its layers
> +   */
> +  attribute boolean isActiveAndForeground;

I think I'd prefer this to be a method vs. a property. Something like SetActiveNoDiscard(). Then we won't have two props for the current active state, which I think confuses things. Most setters will still use the isActive property too, since your use of this is a special case.

The alternative is to update all the uses of isActive to use a method, but I think a special case method for your useis going to be simpler.

::: dom/interfaces/base/nsITabParent.idl
@@ +36,5 @@
> +    * without losing any layer data.
> +    *
> +    * Otherwise, this attribute is the same as above.
> +    */
> +  attribute boolean docShellIsActiveAndForeground;

same here.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 9 years ago

Comment on attachment 8685237 [details] [diff] [review]
0001-Bug-1098131-Don-t-invalidate-layers-when-simply-chan.patch

I don't understand what SetActiveNoDiscard would mean. Docshell doesn't and shouldn't know about layer discarding. 
But a method sounds ok (instead of an attribute)
setIsActiveAndForeground(in bool aActive) ?

	Jim Mathies [:jimm]
	Comment 18 • 9 years ago

I'm ok with the "foreground" text I guess since with docshell it has nothing to do with focus.. focus is what I tend to think of when I read the name. Two getter props though seems wrong since we don't have an internal foreground state that we track.

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

OK, this passes the tests and addresses jimm's review comments.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 20 • 9 years ago

Comment on attachment 8685569 [details] [diff] [review]
0001-Bug-1098131-Don-t-invalidate-layers-when-simply-chan.patch

aHidden isn't perhaps the best param name, but don't have better suggestion now.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/67da9ced67f1

	Carsten Book [:Tomcat]
	Comment 22 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/67da9ced67f1

	Kamil Jozwiak [:kjozwiak]
	Comment 23 • 9 years ago

Reproduced the issue using the following build:
- https://archive.mozilla.org/pub/firefox/nightly/2015/11/2015-11-11-03-02-07-mozilla-central/

Verified using the following build:
- fx 45.0a1 build id: 20151112030238 changeset: 3cc3b1968524
- https://archive.mozilla.org/pub/firefox/nightly/2015/11/2015-11-12-03-02-38-mozilla-central/

Opened several pages and minimized/restored fx and didn't see any "flashes" in the content area. Used google.com, nvidia.com, duckduckgo.com, facebook.com, twitter.com and didn't run into the issue that I reported in bug # 1182841 (dupe of this bug).