Make tab browser warming API not set the docshell to be active

RESOLVED FIXED in Firefox 59

Status

()

P1
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

unspecified
Firefox 59
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 wontfix, firefox59 fixed)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(12 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
In bug 1385453, an API was added to warm up tabs that we think we're likely to switch to. That landed, and great results were seen in Telemetry.

Unfortunately, it also caused a number of regressions. In particular, warming up background tabs caused those tabs to think they were in the foreground (because we were setting the docshell to be active), and that meant it'd do things like:

1) Fire the Page Visibility API stuff for the page JS so that the page thought it was visible (bug 1395735)
2) Unblock paused media - like YouTube videos that had been opened in the background (bug 
1394455)

In bug 1394455, I did some investigation and determined that warming up the tab without setting the docshell as active is possible, but would require some significant changes to TabChild and the async tab switcher (which is already really fragile). Because we were so close to the end of the cycle, I opted to disable tab warming instead, and have filed this bug to do the work necessary to re-enable it.
(Assignee)

Updated

2 years ago
Depends on: 1385453
(Assignee)

Updated

2 years ago
Whiteboard: [reserve-photon-performance][triage]
(Assignee)

Comment 1

2 years ago
In bug 1394455, I found that it was possible to cause a TabChild to paint and upload its layers without activating the DocShell. The steps to do this are very similar to the ones already in InternalSetDocShellIsActive, and will likely need to be factored out into one or more helper methods.

Once that's done, I'll add some IPC messages, "renderLayers" and "clearLayers", which correspond to warming and unwarming a particular TabChild. I think that should be pretty straight forward.

After that, the trick will be to modify the async tab switcher to use this new mechanism. I'm really loathe to add a new state to the tab states - I suspect that will break lots of things. What I think I want to do instead is similar to my original approach to bug 1385453; have a fixed-size collection of warming tabs. When a tab is warmed, we send the renderLayers message for that tab. When we exceed the maximum warming tabs threshold, we call clearLayers on the oldest warmed tab. When a tab is switched to, we remove it from the warming collection. If a tab goes away, we remove it from the warming collection.

If we do it this way, the logic for activating and deactivating the docShells can stay the same.

What do you think of this vague plan, billm? Or should I try adding some kind of STATE_WARMING and STATE_UNWARMING instead?

While I wait to hear back (since billm is on PTO until the 18th), I'll get the native layer patch written up.
Flags: needinfo?(wmccloskey)
Priority: -- → P3
Whiteboard: [reserve-photon-performance][triage] → [reserve-photon-performance]
I guess I was hoping that the tab switcher would only care about the renderLayers/clearLayers stuff. That is, we could just substitute renderLayers/clearLayers for the places where we set docShellIsActive now. Actually setting docShellIsActive could be done as kind of an afterthought, somewhere around here:
http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/base/content/tabbrowser.xml#4558
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 3

2 years ago
I love it - that's how I'll roll. Thanks!
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: P3 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8911917 - Flags: review?(wmccloskey)
Attachment #8911918 - Flags: review?(wmccloskey)
Attachment #8911919 - Flags: review?(wmccloskey)
Attachment #8911920 - Flags: review?(wmccloskey)
(Assignee)

Comment 8

2 years ago
My try push shows these patches crashing all over the place:

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

Clearly not ready for review yet.
Flags: qe-verify? → qe-verify-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

2 years ago
It turns out that the crasher I was dealing with (and have a patch for in this stack) has been fixed by bevis in bug 1399707, so I'll remove that patch from this collection now.
Depends on: 1399707
status-firefox57: --- → wontfix
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8913431 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8913432 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 40

a year ago
mozreview-review
Comment on attachment 8911917 [details]
Bug 1397426 - Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell.

https://reviewboard.mozilla.org/r/183306/#review197416

::: dom/ipc/TabChild.cpp:2600
(Diff revision 6)
> +  if (aIsActive) {
> +    Unused << RecvRenderLayers(true, mLayerObserverEpoch);
> +  } else if (!aPreserveLayers) {
> +    Unused << RecvRenderLayers(false, mLayerObserverEpoch);
> +  }

I don't actually think this is necessary anymore since TabParent::SetDocShellIsActive is calling RenderLayers before sending the SetDocShellIsActive message.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8911917 [details]
Bug 1397426 - Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell.

https://reviewboard.mozilla.org/r/183306/#review200266

Looks good. Thanks.

::: dom/ipc/PBrowser.ipdl:788
(Diff revision 7)
> -     * Update the child side docShell active (resource use) state.
> +     * Update the child side docShell active (resource use) state. If layers
> +     * aren't already being rendered for this child via RenderLayers, they
> +     * will be. Similarly, if a docShell is made inactive and layer rendering
> +     * hasn't already been disabled, this will then disable them (unless
> +     * aPreserveLayers is set to true).

This comment isn't really true. Sending the SetDocShellIsActive message only changes the active state, not the visible state. The TabParent takes care of setting them both, but that's not really relevant in this comment.

::: dom/ipc/TabChild.cpp:2577
(Diff revision 7)
>                                  mPendingDocShellPreserveLayers);
>    }
>  }
>  
>  void
>  TabChild::InternalSetDocShellIsActive(bool aIsActive, bool aPreserveLayers)

Seems like aPreserveLayers is no longer used here.

::: dom/ipc/TabChild.cpp:2587
(Diff revision 7)
> +  if (aIsActive) {
> +    if (!sActiveTabs) {
> +      sActiveTabs = new nsTHashtable<nsPtrHashKey<TabChild>>();
> +    }
> +    sActiveTabs->PutEntry(this);
> +  } else {
> +    if (sActiveTabs) {
> +      sActiveTabs->RemoveEntry(this);
> +      // We don't delete sActiveTabs here when it's empty since that
> +      // could cause a lot of churn. Instead, we wait until ~TabChild.
> +    }
> +  }

This is going to break Quantum DOM, which is the only user of this data structure. It really wants to keep track of the visible tabs rather than the active tabs. Can you rename this (to sVisibleTabs or something) and move the tracking to RenderLayers?

::: dom/ipc/TabChild.cpp:2603
(Diff revision 7)
> +  }
> +}
> +
> +mozilla::ipc::IPCResult
> +TabChild::RecvSetDocShellIsActive(const bool& aIsActive,
> +                                  const bool& aPreserveLayers)

It looks like aPreserveLayers isn't used anywhere in the child. Can we remove it and just track that state in the parent?

::: dom/ipc/TabParent.cpp:2956
(Diff revision 7)
> +
> +  mRenderingLayers = aEnabled;
> +
> +  // Preserve layers means that attempts to stop rendering layers
> +  // will be ignored.
> +  if (!(!aEnabled && mPreserveLayers)) {

Suppose we call RenderLayers(false) when mPreserveLayers is true. Then we call RenderLayers(true). The last call will still ask the child to paint and wait to display the tab until it gets the ForcePaintNoOp message. Could we instead just not send anything and act as if the layers are present? I think you would get that effect if you avoided setting mRenderLayers to false in the RenderLayers(false) call. So maybe you could just move this condition up to the |if (aEnabled == mRenderLayers)| condition? That would also allow you to avoid the double negation, which is a tad confusing.
Attachment #8911917 - Flags: review?(wmccloskey) → review+
Comment on attachment 8911919 [details]
Bug 1397426 - Fix block comment formatting in nsITabParent.idl.

https://reviewboard.mozilla.org/r/183310/#review200808
Attachment #8911919 - Flags: review?(wmccloskey) → review+
Comment on attachment 8911920 [details]
Bug 1397426 - Stop disabling tab warming in browser_bug343515.js.

https://reviewboard.mozilla.org/r/183312/#review200810
Attachment #8911920 - Flags: review?(wmccloskey) → review+
Comment on attachment 8911918 [details]
Bug 1397426 - Make async tab switcher use new nsITabParent renderLayers function.

https://reviewboard.mozilla.org/r/183308/#review200812

::: browser/base/content/tabbrowser.xml:4628
(Diff revision 9)
> +              // If we've reached the point where the requested tab has entered
> +              // the loaded state, but the DocShell is still not yet active, we
> +              // should activate it.
> +              let canCheckDocShellState = !requestedBrowser.isRemoteBrowser ||
> +                                          requestedBrowser.frameLoader.tabParent;
> +
> +              if (requestedTabState == this.STATE_LOADED &&
> +                  canCheckDocShellState &&
> +                  !requestedBrowser.docShellIsActive &&
> +                  !this.minimizedOrFullyOccluded) {
> +                requestedBrowser.docShellIsActive = true;
> +                this.logState("Set requested tab docshell to active");
> +              }

I feel like this should go inside the |if (showTab === this.requestedTab) {| branch (after line 4617). I don't think we should run this code if we're not switching to requestedTab.
Attachment #8911918 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 49

a year ago
mozreview-review-reply
Comment on attachment 8911917 [details]
Bug 1397426 - Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell.

https://reviewboard.mozilla.org/r/183306/#review200266

> This is going to break Quantum DOM, which is the only user of this data structure. It really wants to keep track of the visible tabs rather than the active tabs. Can you rename this (to sVisibleTabs or something) and move the tracking to RenderLayers?

Are you sure we want to do that? At least reading this comment, it seems like GetActiveTabs is supposed to return the tabs that are considered to be "in the foreground". Tabs that are warming shouldn't, I imagine, be included in that set.

Or am I misunderstanding?
(Assignee)

Comment 50

a year ago
mozreview-review-reply
Comment on attachment 8911918 [details]
Bug 1397426 - Make async tab switcher use new nsITabParent renderLayers function.

https://reviewboard.mozilla.org/r/183308/#review200812

> I feel like this should go inside the |if (showTab === this.requestedTab) {| branch (after line 4617). I don't think we should run this code if we're not switching to requestedTab.

That's where I put it originally, but I think there's a problem there - the "blank" case, where the `showTab` becomes the `this.requestedTab` despite us not having gotten layers for it yet.

As I understand it, we have to set the docShell to be active once the layers have finally arrived and the switch has more or less completed. Without that, I was getting some pretty gnarly test failures.

Or am I misunderstanding?
ni?ing for comment 49 and comment 50.
Flags: needinfo?(wmccloskey)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #49)
> Comment on attachment 8911917 [details]
> Bug 1397426 - Add IPC interface to tell TabChild's to render and clear
> layers, distinct from setting the active state on the DocShell.
> 
> https://reviewboard.mozilla.org/r/183306/#review200266
> 
> > This is going to break Quantum DOM, which is the only user of this data structure. It really wants to keep track of the visible tabs rather than the active tabs. Can you rename this (to sVisibleTabs or something) and move the tracking to RenderLayers?
> 
> Are you sure we want to do that? At least reading this comment, it seems
> like GetActiveTabs is supposed to return the tabs that are considered to be
> "in the foreground". Tabs that are warming shouldn't, I imagine, be included
> in that set.
> 
> Or am I misunderstanding?

Yeah, GetActiveTabs should probably be renamed to GetVisibleTabs. It's my fault that the two things got conflated.

The only use of GetActiveTabs is to label the vsync message:
http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/ipc/glue/BackgroundChildImpl.cpp#653
Vsync runs the refresh driver, which processes any tab whose PresShell is active. So the visible tabs need to be on that list.
Flags: needinfo?(wmccloskey)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #50)
> Comment on attachment 8911918 [details]
> Bug 1397426 - Make async tab switcher use new nsITabParent renderLayers
> function.
> 
> https://reviewboard.mozilla.org/r/183308/#review200812
> 
> > I feel like this should go inside the |if (showTab === this.requestedTab) {| branch (after line 4617). I don't think we should run this code if we're not switching to requestedTab.
> 
> That's where I put it originally, but I think there's a problem there - the
> "blank" case, where the `showTab` becomes the `this.requestedTab` despite us
> not having gotten layers for it yet.
> 
> As I understand it, we have to set the docShell to be active once the layers
> have finally arrived and the switch has more or less completed. Without
> that, I was getting some pretty gnarly test failures.
> 
> Or am I misunderstanding?

If we moved the code, wouldn't we still set the docshell to active in the blank case? It seems like all the conditions on those branches would still be true.

I guess it just seems weird that your code is making requestedTab be the active docshell even though it's not clear that showTab == this.requestedTab. If they're not the same, why should we set requestedTab to active yet?
Quick update here - I've been applying the feedback, and have been dealing with some test fallout. This stuff is really touchy, I guess. :/ Hopefully I'll have a new set of patches up today.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 68

a year ago
mozreview-review
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review203260


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/ipc/TabParent.cpp:2962
(Diff revision 1)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +private:
> +  NS_IMETHOD Run() override {

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

Comment 69

a year ago
mozreview-review
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review203262


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/ipc/TabParent.cpp:2963
(Diff revision 2)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +private:
> +  NS_IMETHOD Run() override {

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
(Assignee)

Comment 70

a year ago
mozreview-review-reply
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review203260

> Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

I'm not super sure what clangbot wants me to do here. The `virtual` is coming from the `NS_IMETHOD` macro, and I think I'd rather use the macro than hand-roll the definition of `NS_IMETHOD` without `virtual` for this single usage.

So, uh, sorry clangbot - I think I'm going to drop this (unless this is going to burn me on landing somehow).

Comment 71

a year ago
mozreview-review-reply
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review203260

> I'm not super sure what clangbot wants me to do here. The `virtual` is coming from the `NS_IMETHOD` macro, and I think I'd rather use the macro than hand-roll the definition of `NS_IMETHOD` without `virtual` for this single usage.
> 
> So, uh, sorry clangbot - I think I'm going to drop this (unless this is going to burn me on landing somehow).

Apologies for this unhelpful warning from clangbot, dropping it was the right call.

(In general, clangbot warnings can be safely ignored, and won't burn you on landing in anyway. Unfortunately, these warnings will get reported again for each patch revision, so you may have to drop them multiple times. Sorry about that!)

More in this issue: We've decided to silence defects found inside code that is expanded from macros (because they're usually quite noisy and not helpful, especially when the macro is in third-party code). However, for this particular issue, clang-tidy doesn't give us any indication that the redundant keyword comes from a macro, so clangbot reports it anyway.

Given that the clang-tidy rule "modernize-use-override" causes these problems, and that it's unclear if having it enabled brings any value to our code base, I think we should consider disabling the rule entirely.
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review203882

::: dom/ipc/TabParent.cpp:2975
(Diff revision 2)
> +      // If RenderLayers has been called, and the requested
> +      // rendering state already exists, we don't have to ask
> +      // the TabChild to do anything. Instead, we short-circuit,
> +      // and queue the layer update events for the caller, since
> +      // they wouldn't normally be coming on their own.
> +      RefPtr<LayerTreeUpdateRunnable> runnable =
> +        new LayerTreeUpdateRunnable(this, mLayerTreeEpoch, aEnabled);
> +      NS_DispatchToMainThread(runnable);

Can you explain why this is needed? It makes me worried that the state tracking in tabbrowser.xml is getting screwed up. If that happens, we can end up with races. I'd rather fix the tabbrowser code than add extra checks here.
Comment on attachment 8926449 [details]
Bug 1397426 - Rename TabChild's notion of "active tabs" to "visible tabs" and move logic into renderLayers.

https://reviewboard.mozilla.org/r/197690/#review203884
Attachment #8926449 - Flags: review?(wmccloskey) → review+
Comment on attachment 8926450 [details]
Bug 1397426 - Activate docShells immediately for imageDocument pages when switching tabs with warming enabled.

https://reviewboard.mozilla.org/r/197692/#review203886

I don't get this one either. I'm sorry I didn't ask earlier on IRC. But it's better to have this information in bugs anyway.
(Assignee)

Updated

a year ago
Blocks: 1418099
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 82

a year ago
mozreview-review
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review205656


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/ipc/TabParent.cpp:2963
(Diff revision 3)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +private:
> +  NS_IMETHOD Run() override {

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment hidden (mozreview-request)

Comment 84

a year ago
mozreview-review
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review206056


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/ipc/TabParent.cpp:2963
(Diff revision 3)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +private:
> +  NS_IMETHOD Run() override {

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
(Assignee)

Comment 85

a year ago
mozreview-review-reply
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review203882

> Can you explain why this is needed? It makes me worried that the state tracking in tabbrowser.xml is getting screwed up. If that happens, we can end up with races. I'd rather fix the tabbrowser code than add extra checks here.

We talked about this IRL - basically, there are cases when the selected tab's docshell is inactive, but preserving layers when we're either minimized or fully occluded. When tab switches are initialized in that state, and we set STATE_LOADING on the tab in question, and layers are already available in the compositor, we need the MozLayerTreeReady event to fire again so that the front-end knows to enter STATE_LOADED.

I ended up taking billm's advice here, and only firing this extra MozLayerTreeReady iff we're preserving layers.
Comment hidden (mozreview-request)

Comment 87

a year ago
mozreview-review
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review206110


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/ipc/TabParent.cpp:2963
(Diff revision 3)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +private:
> +  NS_IMETHOD Run() override {

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8929538 [details]
Bug 1397426 - Add hasLayers to nsITabParent and use it in the async tab switcher.

https://reviewboard.mozilla.org/r/200814/#review206478

This looks fine, but I don't think I've reviewed anything that defines renderingLayers. Is that coming up?

::: dom/interfaces/base/nsITabParent.idl:39
(Diff revision 2)
>  
>    /**
> +   * True if layers are being rendered and the compositor has reported
> +   * receiving them.
> +   */
> +  readonly attribute boolean renderingLayers;

I think hasLayers would be a better name here. "rendering" makes it sound like the "renderLayers" call has started and not yet finished.
Attachment #8929538 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 89

a year ago
mozreview-review-reply
Comment on attachment 8929538 [details]
Bug 1397426 - Add hasLayers to nsITabParent and use it in the async tab switcher.

https://reviewboard.mozilla.org/r/200814/#review206478

renderingLayers was defined in the second patch in the series - the one with the commit message, "Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r?billm", which you haven't reviewed yet.

> I think hasLayers would be a better name here. "rendering" makes it sound like the "renderLayers" call has started and not yet finished.

Good idea.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8926450 - Attachment is obsolete: true

Comment 101

a year ago
mozreview-review
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review207522


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/ipc/TabParent.cpp:2963
(Diff revision 4)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +private:
> +  NS_IMETHOD Run() override {

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Gentle review ping.
Flags: needinfo?(wmccloskey)
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review209958

::: dom/ipc/TabParent.cpp:2979
(Diff revision 4)
> +      RefPtr<LayerTreeUpdateRunnable> runnable =
> +        new LayerTreeUpdateRunnable(this, mLayerTreeEpoch, aEnabled);

It might be more concise to do this using a C++ lambda (and then delete LayerTreeUpdateRunnable entirely). Up to you though. Just make sure to keep the TabParent in a RefPtr if you do that though.
Attachment #8926448 - Flags: review?(wmccloskey) → review+
Comment on attachment 8931049 [details]
Bug 1397426 - Rename mRenderingLayers to mRenderLayers.

https://reviewboard.mozilla.org/r/202144/#review209960
Attachment #8931049 - Flags: review?(wmccloskey) → review+
Comment on attachment 8931051 [details]
Bug 1397426 - TabChild::MakeHidden shouldn't cause script to run.

https://reviewboard.mozilla.org/r/202148/#review209968
Attachment #8931051 - Flags: review?(wmccloskey) → review+
Comment on attachment 8931052 [details]
Bug 1397426 - Expose renderLayers state via nsITabParent and lazily retrieve initial tab states in async tab switcher.

https://reviewboard.mozilla.org/r/202150/#review209972

::: browser/base/content/tabbrowser.xml:4480
(Diff revision 1)
> -              let tabIsLoaded = browser.renderingLayers;
> +              for (let tab of this.tabbrowser.tabs) {
> +                if (tab == initialTab && this.minimizedOrFullyOccluded) {
> +                  continue;
> +                }
>  
> -              if (!this.minimizedOrFullyOccluded) {
> +                if (tab.linkedPanel) {

Why are you checking linkedPanel? I don't know what that is.
Attachment #8931052 - Flags: review?(wmccloskey) → review+
Comment on attachment 8931050 [details]
Bug 1397426 - Make TabParent's assume they're rendering layers by default on construction.

https://reviewboard.mozilla.org/r/202146/#review209976
Attachment #8931050 - Flags: review?(wmccloskey) → review+
Sorry, I've been on PTO this week.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 109

a year ago
mozreview-review-reply
Comment on attachment 8931052 [details]
Bug 1397426 - Expose renderLayers state via nsITabParent and lazily retrieve initial tab states in async tab switcher.

https://reviewboard.mozilla.org/r/202150/#review209972

> Why are you checking linkedPanel? I don't know what that is.

We have this thing where tabs can be "discarded", where there's actually no <xul:browser> linked to them yet, and they're linked lazily. That's how restore-on-demand works these days (with great start-up performance gains). We've also exposed an API to WebExtensions for discarding tabs that [people are starting to play with](https://addons.mozilla.org/en-US/firefox/addon/dormancy/).

Accessing the browser immediately causes the browser to be instantiated and inserted into the DOM. The current way to detect the presence of a browser already being in the DOM is to check the true-ness of `linkedPanel`.

So what I'm doing here is avoiding instantiating lazy browsers unnecessarily.
(Assignee)

Comment 110

a year ago
mozreview-review-reply
Comment on attachment 8926448 [details]
Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers.

https://reviewboard.mozilla.org/r/197688/#review209958

> It might be more concise to do this using a C++ lambda (and then delete LayerTreeUpdateRunnable entirely). Up to you though. Just make sure to keep the TabParent in a RefPtr if you do that though.

Good idea, thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 122

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/261be8ec0554
Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell. r=billm
https://hg.mozilla.org/integration/autoland/rev/2cd697170fb3
Rename mRenderingLayers to mRenderLayers. r=billm
https://hg.mozilla.org/integration/autoland/rev/4dd20bf8c0f6
When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r=billm
https://hg.mozilla.org/integration/autoland/rev/4643e46ff8d3
Rename TabChild's notion of "active tabs" to "visible tabs" and move logic into renderLayers. r=billm
https://hg.mozilla.org/integration/autoland/rev/a5b279d39c01
Make async tab switcher use new nsITabParent renderLayers function. r=billm
https://hg.mozilla.org/integration/autoland/rev/5580c145af58
Fix block comment formatting in nsITabParent.idl. r=billm
https://hg.mozilla.org/integration/autoland/rev/e7a6fe9f9ce7
Stop disabling tab warming in browser_bug343515.js. r=billm
https://hg.mozilla.org/integration/autoland/rev/53bb2bc2b676
Add hasLayers to nsITabParent and use it in the async tab switcher. r=billm
https://hg.mozilla.org/integration/autoland/rev/b9b2895b11a3
Make TabParent's assume they're rendering layers by default on construction. r=billm
https://hg.mozilla.org/integration/autoland/rev/78073667ddc6
TabChild::MakeHidden shouldn't cause script to run. r=billm
https://hg.mozilla.org/integration/autoland/rev/8994162ee112
Expose renderLayers state via nsITabParent and correctly set initial tab states in async tab switcher. r=billm
(Assignee)

Updated

a year ago
Depends on: 1423208
(Assignee)

Updated

a year ago
No longer depends on: 1423208
(Assignee)

Updated

a year ago
Depends on: 1423226
I'm hearing enough reports about permanent tab spinners that I'm pretty much ready to back this out.

I'm going to give myself another hour or so to figure it out, and then I'll get a backout happening.
(Assignee)

Updated

a year ago
Depends on: 1423707
Status: RESOLVED → REOPENED
status-firefox59: fixed → ---
Resolution: FIXED → ---
Blocks: 1423220
Just a heads up for the people who are watching, I think I've more or less sorted the intermittent perma-spinners that folks were suffering from due to these patches. Some silly errors on my part, which I'll enumerate when I post a new set of patches.

I'm currently in the process of trying to address the tps Talos regressions that these patches also introduced (bug 1422685).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
These latest patches _should_ address the perma spinners people were experiencing, and should avoid the tps regression (bug 1422685).

A few minor changes:

1) In "When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r=billm", I goofed when applying review feedback, and accidentally cast a uint64_t into a bool when passing some variables to a lambda function. This greatly confused things, and has been fixed.

2) I also got rid of code handling an impossible case where we'd short-circuit and fire MozLayerTreeCleared if we're preserving layers, and unload layers (which can never happen if we're preserving). That has simplified things a bit.

3) I put back the code that made sure that the initial tab had its layers preservation set to false. This was accidentally removed during a refactor.

4) Instead of initializing all of the tab states in the async tab switcher init() function, I do it lazily from within getTabState. This seems to have eliminated the tps regression.

Now pushing to try.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8939692 [details]
Bug 1397426 - Ensure we attempt to activate docShell of current tab after reacting to sidemode or occlusionstate change events.

https://reviewboard.mozilla.org/r/209986/#review216364
Attachment #8939692 - Flags: review?(bill.mccloskey) → review+

Comment 154

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0c38584a789
Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell. r=billm
https://hg.mozilla.org/integration/autoland/rev/7dc566272fee
Rename mRenderingLayers to mRenderLayers. r=billm
https://hg.mozilla.org/integration/autoland/rev/4dd5d1624aba
When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r=billm
https://hg.mozilla.org/integration/autoland/rev/f77d887fffcb
Rename TabChild's notion of "active tabs" to "visible tabs" and move logic into renderLayers. r=billm
https://hg.mozilla.org/integration/autoland/rev/44d6efb9fa69
Make async tab switcher use new nsITabParent renderLayers function. r=billm
https://hg.mozilla.org/integration/autoland/rev/d39683d8f79f
Fix block comment formatting in nsITabParent.idl. r=billm
https://hg.mozilla.org/integration/autoland/rev/f6e07e976045
Stop disabling tab warming in browser_bug343515.js. r=billm
https://hg.mozilla.org/integration/autoland/rev/3fa35817d29d
Add hasLayers to nsITabParent and use it in the async tab switcher. r=billm
https://hg.mozilla.org/integration/autoland/rev/707a2b0aede1
Make TabParent's assume they're rendering layers by default on construction. r=billm
https://hg.mozilla.org/integration/autoland/rev/8605e2ebcc92
TabChild::MakeHidden shouldn't cause script to run. r=billm
https://hg.mozilla.org/integration/autoland/rev/c592bf74cad1
Expose renderLayers state via nsITabParent and lazily retrieve initial tab states in async tab switcher. r=billm
https://hg.mozilla.org/integration/autoland/rev/2cfdaedf6359
Ensure we attempt to activate docShell of current tab after reacting to sidemode or occlusionstate change events. r=billm

Comment 156

a year ago
sorry to be a downer, but I am getting the infinite spinning wheel on tabs.
Flags: needinfo?(mconley)

Updated

a year ago
Depends on: 1428604
(Assignee)

Updated

a year ago
No longer depends on: 1428604

Updated

a year ago
Flags: needinfo?(mconley)
Blocks: 1430153
(Assignee)

Updated

a year ago
Blocks: 1430292

Updated

9 months ago
Blocks: 1472230
You need to log in before you can comment on or make changes to this bug.