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

REOPENED
Assigned to

Status

()

Core
Layout
P2
normal
REOPENED
3 months ago
15 days ago

People

(Reporter: dbaron, Assigned: dbaron)

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

3 months 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

3 months 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

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

Comment 3

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → dbaron
(Assignee)

Comment 16

3 months 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

3 months 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

3 months ago
status-firefox55: fixed → ---
status-firefox57: affected → ---
Target Milestone: mozilla55 → ---
(Assignee)

Comment 18

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

Comment 19

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

Comment 20

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

Comment 21

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

Comment 22

2 months 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)

Comment 23

a month ago
Sorry for delay in answering. It makes sense to disable all tests but openWndShutdownDoc in events/test_docload.html to reduce the noise, and then add lines:

gA11yEventDumpToConsole = true;
enableLogging("tree,doclifecycle");

This will provide a log that may explain why we get second reorder event on a root accessible.
Flags: needinfo?(surkov.alexander)

Updated

a month ago
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.