Closed Bug 1098131 Opened 5 years ago Closed 4 years ago

[e10s] Content area flashes white when browser window is un-minimized


(Core :: General, defect, minor)

Not set



Tracking Status
e10s m8+ ---
firefox45 --- fixed


(Reporter: jruderman, Assigned: gw280)




(1 file, 2 obsolete files)

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".]
Assignee: nobody → gwright
I'm hoping this is a dupe of bug 1157941
Duplicate of this bug: 1182841
Duplicate of this bug: 1187001
Using paint flashing, I see two invalidations between the before-minimize and after-unminimize states. But I don't see any white flashing.
So it's coming from here:

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.
> // 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?
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.
Attachment #8681673 - Flags: review?(bugs)
Comment on attachment 8681673 [details] [diff] [review]

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

>+nsDocShell::SetIsActive(bool aIsActive) {

>+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.)
Attachment #8681673 - Flags: review?(bugs) → review+
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.
Backed out by 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]
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.
Attachment #8681673 - Attachment is obsolete: true
Attachment #8685237 - Flags: review?(jmathies)
Attachment #8685237 - Flags: review?(bugs)
Comment on attachment 8685237 [details] [diff] [review]

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.
Attachment #8685237 - Flags: review?(jmathies) → review-
Comment on attachment 8685237 [details] [diff] [review]

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) ?
Attachment #8685237 - Flags: review?(bugs)
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.
OK, this passes the tests and addresses jimm's review comments.
Attachment #8685237 - Attachment is obsolete: true
Attachment #8685569 - Flags: review?(bugs)
Comment on attachment 8685569 [details] [diff] [review]

aHidden isn't perhaps the best param name, but don't have better suggestion now.
Attachment #8685569 - Flags: review?(bugs) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Reproduced the issue using the following build:

Verified using the following build:
- fx 45.0a1 build id: 20151112030238 changeset: 3cc3b1968524

Opened several pages and minimized/restored fx and didn't see any "flashes" in the content area. Used,,,, and didn't run into the issue that I reported in bug # 1182841 (dupe of this bug).
Depends on: 1227373
Depends on: 1234049
Depends on: 1226948
You need to log in before you can comment on or make changes to this bug.