mark hidden window as inactive (so we will soon not run the refresh driver in the hidden window)

REOPENED
Assigned to

Status

()

Core
Layout
REOPENED
a month ago
10 hours ago

People

(Reporter: dbaron, Assigned: dbaron, NeedInfo)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a month ago
Right now we run the refresh driver in the hidden window, which causes some (reasonably simple) reflows during startup.

We should also ensure that PresShell::SetIsActive(false) happens on the hidden window's pres shell.

(But see also bug 71895.)
(Assignee)

Comment 1

a month ago
A naive, untested patch to mark it as inactive is https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/9ee0e6dc1793/mark-hidden-window-inactive
(Assignee)

Comment 2

a month ago
(I'm not planning to work on this further now.)
See Also: → bug 1352205
(Assignee)

Comment 3

a month ago
On the other hand, if we do bug 1352205 in full (which is sounding likely), this patch might be all we need.
(Assignee)

Comment 4

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbd1dd4d094c4f7529912ac1778777ce1df17fc9&group_state=expanded
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a month ago
mozreview-review
Comment on attachment 8871536 [details]
Bug 1367830 - Refactor to share more code between private and non-private hidden window creation.

https://reviewboard.mozilla.org/r/143008/#review147048

::: xpfe/appshell/nsAppShellService.cpp:144
(Diff revision 2)
> -    rv = JustCreateTopWindow(nullptr, url,
> +  rv = JustCreateTopWindow(nullptr, url,
> -                             chromeMask, initialWidth, initialHeight,
> +                           chromeMask, initialWidth, initialHeight,
> -                             true, nullptr, nullptr, getter_AddRefs(newWindow));
> +                           true, nullptr, nullptr, getter_AddRefs(newWindow));
> -    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
>  
> +  {

Is it necessary to Release() the docShell before swapping the private window? This extra scope seems ugly & unnecessary.

::: xpfe/appshell/nsAppShellService.cpp:147
(Diff revision 2)
>      if (docShell) {
> +      if (aIsPrivate) {

This is really ugly - I would ask you to merge into `if (aIsPrivate && docShell)` except that you need the `docShell` for non-private windows in part 2.

::: xpfe/appshell/nsAppShellService.cpp:154
(Diff revision 2)
> -      docShell->SetAffectPrivateSessionLifetime(false);
> +        docShell->SetAffectPrivateSessionLifetime(false);
> -    }
> +      }
> +    }
> +  }
>  
> +  if (!aIsPrivate) {

Can we flip this if statement? Other if statements here use (aIsPrivate) not (!aIsPrivate)

::: xpfe/appshell/nsAppShellService.cpp:157
(Diff revision 2)
> +  }
>  
> +  if (!aIsPrivate) {
> +    mHiddenWindow.swap(newWindow);
> +  } else {
> +    // We created the hidden private window

This comment feels silly here

::: xpfe/appshell/nsAppShellService.cpp:161
(Diff revision 2)
> +  } else {
> +    // We created the hidden private window
>      mHiddenPrivateWindow.swap(newWindow);
>    }
>  
>    // RegisterTopLevelWindow(newWindow); -- Mac only

This isn't an issue with your patch, but I still find this very confusing. What is the purpose of this comment? It appears from blame to have just floated around here since the netscape days.
Attachment #8871536 - Flags: review?(michael) → review+

Comment 10

a month ago
mozreview-review
Comment on attachment 8871537 [details]
Bug 1367830 - Mark hidden window as inactive.

https://reviewboard.mozilla.org/r/143010/#review147054
Attachment #8871537 - Flags: review?(michael) → review+
(Assignee)

Updated

a month ago
Summary: don't run the refresh driver in the hidden window → mark hidden window as inactive (so we will soon not run the refresh driver in the hidden window)
(Assignee)

Comment 11

a month ago
mozreview-review
Comment on attachment 8871536 [details]
Bug 1367830 - Refactor to share more code between private and non-private hidden window creation.

https://reviewboard.mozilla.org/r/143008/#review147064

::: xpfe/appshell/nsAppShellService.cpp:144
(Diff revision 2)
> -    rv = JustCreateTopWindow(nullptr, url,
> +  rv = JustCreateTopWindow(nullptr, url,
> -                             chromeMask, initialWidth, initialHeight,
> +                           chromeMask, initialWidth, initialHeight,
> -                             true, nullptr, nullptr, getter_AddRefs(newWindow));
> +                           true, nullptr, nullptr, getter_AddRefs(newWindow));
> -    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
>  
> +  {

Not particularly, but it seemed like a good idea; often leads to less destructor code to keep scopes small, although it doesn't particularly matter here.  I guess I'll drop it.

::: xpfe/appshell/nsAppShellService.cpp:147
(Diff revision 2)
>      if (docShell) {
> +      if (aIsPrivate) {

Yeah, I did it this way so I wouldn't have to change it again in part 2.

::: xpfe/appshell/nsAppShellService.cpp:154
(Diff revision 2)
> -      docShell->SetAffectPrivateSessionLifetime(false);
> +        docShell->SetAffectPrivateSessionLifetime(false);
> -    }
> +      }
> +    }
> +  }
>  
> +  if (!aIsPrivate) {

Sure.

::: xpfe/appshell/nsAppShellService.cpp:157
(Diff revision 2)
> +  }
>  
> +  if (!aIsPrivate) {
> +    mHiddenWindow.swap(newWindow);
> +  } else {
> +    // We created the hidden private window

Yeah, I was trying to preserve the old comment, but I'll drop it.

::: xpfe/appshell/nsAppShellService.cpp:161
(Diff revision 2)
> +  } else {
> +    // We created the hidden private window
>      mHiddenPrivateWindow.swap(newWindow);
>    }
>  
>    // RegisterTopLevelWindow(newWindow); -- Mac only

Oops, meant to delete this comment, but forgot.  I'll do so.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a month ago
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48ee194df638
Refactor to share more code between private and non-private hidden window creation.  r=mystor
https://hg.mozilla.org/integration/autoland/rev/d706d716fcbb
Mark hidden window as inactive.  r=mystor
https://hg.mozilla.org/mozilla-central/rev/48ee194df638
https://hg.mozilla.org/mozilla-central/rev/d706d716fcbb
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → dbaron
(Assignee)

Comment 16

27 days ago
I've been meaning to follow up here:  this didn't actually do much because it doesn't actually propagate to the PresShell for some reason; I've been meaning to debug that.
(Assignee)

Comment 17

27 days ago
That's because of the test at the start of nsDocShell::SetIsActive.  Basically, the patches here so far haven't done anything yet.  (I'd meant to test, but it managed to drop off my list of things to do during the QF work week...)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

27 days ago
status-firefox55: fixed → ---
status-firefox57: affected → ---
Target Milestone: mozilla55 → ---
(Assignee)

Comment 18

27 days ago
With that removed, I no longer see reflows in the hidden window triggered from vsync refresh driver timer ticks.
(Assignee)

Comment 19

27 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1d532de9ec133c2a9b5798ddea5b7d68704f36f&group_state=expanded
(Assignee)

Comment 20

22 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a40577ecc28380c1ea78e8613fe31e913eb4549b&group_state=expanded
(Assignee)

Comment 21

22 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dbadcfa26606ea2be4df9dc81609d1a2124564e&group_state=expanded
(Assignee)

Comment 22

10 hours ago
So my patch here is causing a single mochitest failure (visible in the above try runs):

 FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_docload.html | Scenario #0 of test with ID = 'open dialog '../events/docload_wnd.html'' failed. Dupe reorder event.

From the history it looks like this is something that's been worked around before.  I'm curious if you have any advice on how to approach investigating this failure -- either adjusting the test, or figuring out whether there's a real problem as a result of the hidden window being inactive and having a slower refresh driver cycle.
Flags: needinfo?(surkov.alexander)
You need to log in before you can comment on or make changes to this bug.