Prepare the main view just before opening PanelMultiView panels

RESOLVED FIXED in Firefox 60

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
10 days ago
6 days ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

59 Branch
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

10 days ago
Preparing the main view just before opening the panel allows the ViewShowing event to prevent the panel from opening. It also avoids setting up the main view if the panel is not opened.
(Assignee)

Comment 1

8 days ago
It was quite time consuming, but I was eventually able to track the leak from bug 1434883 down to a single test that didn't wait for the panel events. I'm now continuing on this track since bug 1437512 is currently blocked on bug 1434376.
No longer depends on: 1437512
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

8 days ago
mozreview-review
Comment on attachment 8951234 [details]
Bug 1437811 - Part 1 - Fix test that didn't wait for PanelMultiView events.

https://reviewboard.mozilla.org/r/220504/#review226510

rs=me
Attachment #8951234 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 6

8 days ago
mozreview-review
Comment on attachment 8951235 [details]
Bug 1437811 - Part 2 - Prepare the main view just before opening the panel.

https://reviewboard.mozilla.org/r/220506/#review226512

Is it just me or did I already review this in another bug?

::: browser/components/customizableui/PanelMultiView.jsm:488
(Diff revision 1)
>      this.showSubView(current, null, previous);
>    }
>  
>    showMainView() {
>      if (!this.node || !this._mainViewId)
> -      return Promise.resolve();
> +      return Promise.resolve(false);

Could just mark this async and `return false` ?

::: browser/components/customizableui/PanelMultiView.jsm:933
(Diff revision 1)
> +        if (this._viewShowing) {
> +          PanelView.forNode(this._viewShowing).dispatchCustomEvent("ViewHiding");
> +        }

Is this relevant to the previous comment? What in particular is it doing? Are we just sending ViewHiding events where we didn't before?
Attachment #8951235 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 7

8 days ago
mozreview-review
Comment on attachment 8951236 [details]
Bug 1437811 - Part 3 - Add a safety timeout for blockers registered by event handlers.

https://reviewboard.mozilla.org/r/220508/#review226520

Tentative r=me in that this code seems OK for what it does, but can you elaborate on why this was necessary?

::: browser/components/customizableui/PanelMultiView.jsm:197
(Diff revision 1)
> +        try {
> +          let results = await Promise.race([Promise.all(blockers),
> +                                            timeoutPromise]);
> +          cancel = cancel || results.some(result => result === false);
> +        } catch (ex) {
> +          Cu.reportError(`One of the blockers for ${eventName} timed out.`);

Nit: new Error
Attachment #8951236 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 8

7 days ago
(In reply to :Gijs from comment #6)
> Is it just me or did I already review this in another bug?

Right, except the later change to make tests pass.

> > +        if (this._viewShowing) {
> > +          PanelView.forNode(this._viewShowing).dispatchCustomEvent("ViewHiding");
> > +        }
> 
> Is this relevant to the previous comment? What in particular is it doing?
> Are we just sending ViewHiding events where we didn't before?

So... I thought I had a straightforward explanation, in that we need to ensure that we send the same events as we did before for the case where the entire panel is hidden during the ViewShowing event, but I did some event tracing while running browser_ext_browserAction_popup.js and the situation here is quite complicated.

In fact, the call to showMainView in the "popuphidden" handler caused hideAllViewsExcept to be called at least two times. The first happens inside _cleanupTransitionPhase using the values of the transition that is currently in progress. The second is called directly by showMainView with the main view as the current view. I also think it can be called a third time afterwards, when the current transition finishes or is cancelled.

Calls to hideAllViewsExcept not only trigger ViewHiding events, but also new ViewShown events. It's quite possible we do a bunch of unnecessary work in this stage, at least in the test cases, by constructing and destructing WebExtension panels. The WebExtensions code gets confused if events run in a slightly different order, so touching anything there is quite difficult.

I think this bug can now land, since manual testing shows no issue and automated tests pass, but I should probably look into making hidePopup calls from within event handlers non-reentrant.
(Assignee)

Comment 9

7 days ago
(In reply to :Gijs from comment #7)
> Tentative r=me in that this code seems OK for what it does, but can you
> elaborate on why this was necessary?

The openPopup call now waits until we have processed the ViewShowing events for the main view. Since openPopup is non-reentrant, a single locked blocker invocation could prevent the panel from opening for the rest of the session. With this change, we'll at least retry later. I think you suggested implementing this protection in a previous review.

I don't think we register blockers for the main view, but this will apply to more cases once we make showSubView non-reentrant.
(Assignee)

Comment 10

7 days ago
(In reply to :Gijs from comment #7)
> ::: browser/components/customizableui/PanelMultiView.jsm:197
> (Diff revision 1)
> > +        try {
> > +          let results = await Promise.race([Promise.all(blockers),
> > +                                            timeoutPromise]);
> > +          cancel = cancel || results.some(result => result === false);
> > +        } catch (ex) {
> > +          Cu.reportError(`One of the blockers for ${eventName} timed out.`);
> 
> Nit: new Error

Does this improve debugging? I tried and what I see in the Browser Console is quite similar.

Comment 11

7 days ago
(In reply to :Paolo Amadini from comment #10)
> (In reply to :Gijs from comment #7)
> > ::: browser/components/customizableui/PanelMultiView.jsm:197
> > (Diff revision 1)
> > > +        try {
> > > +          let results = await Promise.race([Promise.all(blockers),
> > > +                                            timeoutPromise]);
> > > +          cancel = cancel || results.some(result => result === false);
> > > +        } catch (ex) {
> > > +          Cu.reportError(`One of the blockers for ${eventName} timed out.`);
> > 
> > Nit: new Error
> 
> Does this improve debugging? I tried and what I see in the Browser Console
> is quite similar.

Yes, it used to be that you got a stack for one and not for the other. It's still good practice, as far as I'm aware.

Comment 12

7 days ago
(In reply to :Paolo Amadini from comment #8)
> Calls to hideAllViewsExcept not only trigger ViewHiding events, but also new
> ViewShown events. It's quite possible we do a bunch of unnecessary work in
> this stage, at least in the test cases, by constructing and destructing
> WebExtension panels. The WebExtensions code gets confused if events run in a
> slightly different order, so touching anything there is quite difficult.
> 
> I think this bug can now land, since manual testing shows no issue and
> automated tests pass, but I should probably look into making hidePopup calls
> from within event handlers non-reentrant.

But you're explicitly touching this code and causing some of these calls. Is the situation better with your patch, or worse? I don't think it's a good idea to just be firing more events now, and I don't really understand what this:

(In reply to :Paolo Amadini from comment #8)
> we need to
> ensure that we send the same events as we did before for the case where the
> entire panel is hidden during the ViewShowing event

Especially because my understanding is that we're now not showing the panel at all if this happens.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 13

7 days ago
(In reply to :Gijs from comment #12)
> But you're explicitly touching this code and causing some of these calls. Is
> the situation better with your patch, or worse? I don't think it's a good
> idea to just be firing more events now

Well, the point is that we are already firing quite a lot of events in this case. This hasn't been designed this way, it just happened to work and tests passed. We have to keep the tests passing and the product working in order to land incremental improvements, which is what I'm trying to do.

It's actually possible that the issue I observed before with a missing ViewHiding event was caused by me accidentally swapping the order of "this._cleanupTransitionPhase();" and "this.hideAllViewsExcept(null);" in my patch compared to what showMainView was doing. I'm running a tryserver build to see if this was the issue - in this case, we can definitely remove the explicit dispatch of ViewHiding for the currently showing view, and keep it implicit in the way hideAllViewsExcept calls are made. To answer your question, it's not better or worse - it's a mess either way.

However, if this is not the issue, I think it's better to dispatch ViewHiding manually instead of doing it implicitly because showMainView is calling hideAllViewsExcept for the main view.

Also note that we probably don't want to raise ViewShown events for the main view when the panel is closed, but only when it's opened and the main view is actually visible. I'm not sure we're there yet, because _cleanupTransitionPhase can still raise ViewShown events for the current transition as a side-effect.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 14

7 days ago
(In reply to :Paolo Amadini from comment #13)
> It's actually possible that the issue I observed before with a missing
> ViewHiding event was caused by me accidentally swapping the order of
> "this._cleanupTransitionPhase();" and "this.hideAllViewsExcept(null);"

Looks like so, I'll go ahead and land the amended version once more tests in the tryserver build are green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18dfa0ecb15fb396d17e7b1e20e2e2efd5d16acf&selectedJob=162662334

Comment 15

6 days ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2948cdf0a5
Part 1 - Fix test that didn't wait for PanelMultiView events. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/57250accd697
Part 2 - Prepare the main view just before opening the panel. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d8569f3a93
Part 3 - Add a safety timeout for blockers registered by event handlers. r=Gijs

Comment 16

6 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd2948cdf0a5
https://hg.mozilla.org/mozilla-central/rev/57250accd697
https://hg.mozilla.org/mozilla-central/rev/c3d8569f3a93
Status: ASSIGNED → RESOLVED
Last Resolved: 6 days ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.