Closed Bug 1434883 Opened 6 years ago Closed 6 years ago

Make panel opening in PanelMultiView asynchronous

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

Product:
Firefox
Component:
Toolbars and Customization
Version:
59 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1434883 - Part 1 - Fix tests for the activated page action panel.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1434883 - Part 2 - Use an asynchronous API to open PanelMultiView panels.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
In order to make the popuphidden handling in PanelMultiView completely synchronous, we should make the popup opening asynchronous. This allows completing any pending operations and canceling animations on the new main view.

This will also enable several improvements:
* Set up the main view lazily even when the binding is already attached
* Allow the ViewShowing event for the main view to cancel opening
* Apply an asynchronous descriptionHeightWorkaround for the main view
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1390040
I've posted my progress so far, although the work is not finished yet.

Part 1 and 2 already pass all tests, meaning that at least conceptually we fixed all the cases where opening the view wasn't fully asynchronous. This by itself was definitely less work than I expected, our tests were in good shape already, especially for the identity popup.

For the identity popup, it's also worth noting that the only asynchronous operation is still the panel opening, which is already done asynchronously by the platform. Both updating the elements inside the panel and closing the panel are synchronous operations. This means there are no new concerns about displaying stale information in the popup or delaying closing it on navigation.

I'm running tests for part 3 right now to see if anything breaks because of those specific changes, or needs changing.

I'll write a Part 4 because, as we discussed before, we don't want potentially unbound ViewShowing blockers, so I'm thinking of adding a safety timeout of 15 seconds. As far as I know the only remaining handlers that add a blocker are the browser action panels, that should unblock almost instantly, and the Downloads Panel, that may block if loading the data from disk takes a long time. The latter is the only case where the timeout is likely to occur in practice. Other cases can only be coding errors that prevent the asynchronous function from returning at all, so 15 seconds should be enough just to avoid blocking the panel for the rest of the session.
Comment on attachment 8947517 [details]
Bug 1434883 - Part 1 - Fix tests for the activated page action panel.

https://reviewboard.mozilla.org/r/217222/#review223222

::: browser/base/content/test/urlbar/head.js:272
(Diff revision 1)
>  function promisePanelEvent(panelIDOrNode, eventType) {
>    return new Promise(resolve => {
>      let panel = typeof(panelIDOrNode) != "string" ? panelIDOrNode :
>                  document.getElementById(panelIDOrNode);
> -    if (!panel ||
> -        (eventType == "popupshown" && panel.state == "open") ||
> +    if (!panel) {
> +      throw new Error("The specified panel does not exist.");

Nit: use a template string and include `${panelIDOrNode}` so that we can see which ID is missing if this ever fails when writing tests / intermittently / whatever. :-)

::: browser/modules/test/browser/browser_PageActions.js:1469
(Diff revision 1)
>  function promisePanelEvent(panelIDOrNode, eventType) {
>    return new Promise(resolve => {
>      let panel = typeof(panelIDOrNode) != "string" ? panelIDOrNode :
>                  document.getElementById(panelIDOrNode);
> -    if (!panel ||
> -        (eventType == "popupshown" && panel.state == "open") ||
> +    if (!panel) {
> +      throw new Error("The specified panel does not exist.");

Ditto.
Attachment #8947517 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8947518 [details]
Bug 1434883 - Part 2 - Use an asynchronous API to open PanelMultiView panels.

https://reviewboard.mozilla.org/r/217224/#review223224

::: browser/components/customizableui/PanelMultiView.jsm:256
(Diff revision 1)
>     */
>    get currentShowPromise() {
>      return this._currentShowPromise || Promise.resolve();
>    }
>  
> +  // The constructor may be called before the XBL binding is connected.

"may be" or "is" ? Why is this not deterministic?

::: browser/components/customizableui/PanelMultiView.jsm:370
(Diff revision 1)
> +    let cancel = false;
> +    this._openPopupCancelCallback = () => {
> +      // This races with the panel opening below.
> +      if (!cancel && this.node) {
> +        cancel = true;
> +        this.dispatchCustomEvent("popuphidden");

This feels smelly. Why do we need to manually dispatch this? Why don't we just call hidePopup() ? What does manually dispatching this accomplish - does it actually hide the popup, or not really, or...?

::: browser/components/customizableui/PanelMultiView.jsm:376
(Diff revision 1)
> +      // Identify issues with regression tests.
> +      await new Promise(resolve => this.window.setTimeout(resolve, 50));

So do you intend to remove this? Why is this still here?

::: browser/components/customizableui/PanelMultiView.jsm:379
(Diff revision 1)
> +      if (!cancel && this.node) {
> +        cancel = true;
> +        this._panel.openPopup(...args);
> +      }

The fact that both of these manipulate a value called `cancel` but one of them is a `CancelCallback` and the other the opening promise resolution handler is confusing. Can we come up with a better name for `cancel` ?

::: browser/components/customizableui/PanelMultiView.jsm:385
(Diff revision 1)
> +        cancel = true;
> +        this._panel.openPopup(...args);
> +      }
> +    });
> +    // We don't report errors here because they will be caught by caller.
> +    this._openPopupPromise = newOpenPopupPromise.catch(() => {});

This is very confusing. If you're catching them here, how are you catching them in the caller?
Attachment #8947518 - Flags: review?(gijskruitbosch+bugs) → review-
Comment on attachment 8947519 [details]
Bug 1434883 - Part 3 - Prepare the main view just before opening the panel.

https://reviewboard.mozilla.org/r/217226/#review223312

::: browser/components/customizableui/PanelMultiView.jsm:358
(Diff revision 1)
>      this._openPopupCancelCallback = () => {
>        // This races with the panel opening below.
>        if (!cancel && this.node) {
>          cancel = true;
>          this.dispatchCustomEvent("popuphidden");
>        }
>      };

I have to admit I don't understand how this helps. This doesn't seem to resolve the open promise, so won't subsequent opening promises just wait forever because they're `.then`'d on the previous open promise?

::: browser/components/customizableui/PanelMultiView.jsm:368
(Diff revision 1)
> -      // Identify issues with regression tests.
> -      await new Promise(resolve => this.window.setTimeout(resolve, 50));
> +      // Allow any of the ViewShowing handlers to prevent showing the main view.
> +      let shown = await this.showMainView();

Ah. So I would merge the removal of the 50ms await here into the previous cset...

::: browser/components/customizableui/PanelMultiView.jsm:375
(Diff revision 1)
> +          return true;
> -      }
> +        }
> +      }
> +      return false;

consolidate these two into `return !!shown` ?

::: browser/components/customizableui/PanelMultiView.jsm:382
(Diff revision 1)
> +      }
> +      return false;
>      });
>      // We don't report errors here because they will be caught by caller.
>      this._openPopupPromise = newOpenPopupPromise.catch(() => {});
> -    await newOpenPopupPromise;
> +    return newOpenPopupPromise;

This should probably also be merged into the previous change.

::: browser/components/customizableui/content/panelUI.js:425
(Diff revision 1)
>        let oldMultiView = viewNode.panelMultiView;
>        if (oldMultiView && oldMultiView.current == viewNode) {
>          await oldMultiView.showMainView();
>        }
>  
>        let viewShown = false;

Please move this right before the `panelRemover` definition.

::: browser/components/customizableui/content/panelUI.js:469
(Diff revision 1)
> +      if (!viewShown) {
> +        panelRemover();
> +        return;
> +      }
> +
> +      CustomizableUI.addPanelCloseListeners(tempPanel);
> +      tempPanel.addEventListener("popuphidden", panelRemover);

I think this would be clearer if it was just an `if (viewShown) { ... } else  { panelRemover(); }`, even if they're functionally equivalent.
Attachment #8947519 - Flags: review?(gijskruitbosch+bugs)
I have rewritten most of Part 2 and added more comments, hopefully it's less confusing but let me know if some elements are still unclear.

(In reply to :Gijs from comment #6)
> > +        this.dispatchCustomEvent("popuphidden");
> 
> This feels smelly. Why do we need to manually dispatch this? Why don't we
> just call hidePopup() ? What does manually dispatching this accomplish -
> does it actually hide the popup, or not really, or...?

This is in fact the trickiest element of the patch. For what I'm trying to accomplish here, I am still undecided on whether to use "popuphidden", "PanelMultiViewHidden", or a new event like "popupshowingcanceled", and there is not a perfect solution. I've also considered only resolving the returned Promise when the panel is closed, but this doesn't account for re-entering the method.

The consumers of the openPopup method generally expect that if the function returns successfully, then the panel will be displayed until the "panelhidden" method is fired, so they will do some cleanup in that event handler. This is the general pattern for panels that are not PanelMultiView. An example of cleanup is removing the "open" state on the anchor.

With the changes in Part 2, some time may pass between the openPopup call and the actual panel opening, while the main view is being prepared. This was already taken into account manually by consumers like PanelUI.js, but is now true for all consumers.

With the changes in Part 3, the opening operation can be canceled from ViewShowing events. In this case, the "panelhidden" event will never happen. If this is the event used to clear the anchor state, the anchor will keep the "open" state indefinitely.

In practice, I'm looking for the best way to support pseudo-code like:

panelMultiView.on(event, anchor.open = false);
anchor.open = true;
panelMultiView.openPopup();

Any suggestions?
I found something really interesting by checking that the binding was connected:

https://reviewboard.mozilla.org/r/217224/diff/2-3#0.6

This means that, previously, showing a panel for the first time and constructing the binding would run in parallel, possibly racing with each other and unable to guarantee the exact sequence of panel showing events and view showing events.

In general, this may be a source of intermittent failures for panels that have XBL bindings inside them, even without using PanelMultiView. I think we've known this general issue for a long time, and hopefully with the de-XBL effort and these investigations we may be able to make the situation better.
I realized there was some confusion in the documentation between turning on the display of a panel by removing the "hidden" attribute and actually opening a panel. I clarified this and also removed some duplication, as the comments for openPopup and hidePopup were becoming quite complex. There is still a _lot_ of plumbing required for asynchronous programming and especially when XBL is involved...
(In reply to :Paolo Amadini from comment #11)
> Any suggestions?

Well, I see two options:
1) fire popuphidden like the previous patch
2) fire a separate event and update callsites to add listeners for the other event, too.


The downside of the first option is:
a) popuphidden may fire unexpectedly (when popupshown/popuppositioned hasn't fired) and so might throw off code - but consumer code can trivially determine when this happens and deal with it. In practice I doubt this will be a significant issue.
b) it's confusing to look at the implementation code (why are we randomly and manually firing this event here?).
c) in theory it could confuse internal code if it ever started to hook popup things on that event.

The downside of the second option is:

a) you have to register the same listener for 2 events
b) you have to remove the listener for both events - unlike for a single event where you can use {once: true}, there's no builtin way to say "do this once for any of these events".
c) you have to update all the existing callsites to deal with the second event
d) you have to teach any people who write new code that uses this stuff about the second event, and the obvious question is going to be "why do we even have 2 events".

I think the first option is probably better. But in the patch you went with using panelmultiviewhidden. Why?
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8947518 [details]
Bug 1434883 - Part 2 - Use an asynchronous API to open PanelMultiView panels.

https://reviewboard.mozilla.org/r/217224/#review223754

::: browser/components/customizableui/PanelMultiView.jsm:62
(Diff revisions 1 - 4)
> +ChromeUtils.defineModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm");

Just use ChromeUtils.import(). No point lazily importing Services.jsm.

::: browser/components/customizableui/PanelMultiView.jsm:363
(Diff revisions 1 - 4)
> -
> -    let cancel = false;
> -    this._openPopupCancelCallback = () => {
> +      // The earliest between a cancellation request and a successful panel
> +      // preparation will prevent the PanelMultiViewHidden event from being
> +      // dispatched here.

Nit: "The earliest" isn't quite English. Perhaps:

```
// If the cancel callback is called and the panel hasn't been prepared yet,
// cancel showing it. Setting `canCancel` will prevent the popup from opening.
// If the panel has opened by the time the cancel callback is called, `canCancel`
// will be false and we will not fire the PanelMultiViewhidden event.
```
(or the popuphidden event, given my comment on the bug, but ymmv).
Attachment #8947518 - Flags: review?(gijskruitbosch+bugs) → review+
Actually, for comment #18, a third option would be pushing the anchor functionality into PMV... just make callsites set the id of the anchor as an attribute on the <pmv> and deal with it internally. That avoids the custom event stuff and is probably cleaner. This assumes that there aren't other reasons we want to call popuphidden/pmvhidden event listeners though...
Comment on attachment 8947519 [details]
Bug 1434883 - Part 3 - Prepare the main view just before opening the panel.

https://reviewboard.mozilla.org/r/217226/#review223758
Attachment #8947519 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #18)
> I think the first option is probably better. But in the patch you went with
> using panelmultiviewhidden. Why?

I wanted to see whether automation had a preference between the two :-) It seems that tests pass either way though, so the "popuphidden" may indeed be the best at the moment without changing the callers.

(In reply to :Gijs from comment #20)
> Actually, for comment #18, a third option would be pushing the anchor
> functionality into PMV... just make callsites set the id of the anchor as an
> attribute on the <pmv> and deal with it internally. That avoids the custom
> event stuff and is probably cleaner. This assumes that there aren't other
> reasons we want to call popuphidden/pmvhidden event listeners though...

If we modify the callers, the best option would be to wrap the entire operation in PanelMultiViewShowing and PanelMultiViewHidden events that are always called in pairs, and can be used to control the anchor or other states. Multiple calls to openPopup would result in only one PanelMultiViewShowing event. So the process would look like:

 - PanelMultiViewShowing
 - ViewShowing (can delay or cancel the operation)
    - popupshowing
    - popupshown and ViewShown (in any order)
    - popuphiding and ViewHiding (in any order)
    - popuphidden
 - ViewHidden
 - PanelMultiViewHidden

Note that there is currently code relying on ViewHiding being fired even if ViewShowing cancels the operation. It makes sense to have an event that behaves like this, as there may be multiple ViewShowing event handlers and the cancellation may be triggered by a different handler. However, this event should probably be ViewHidden, while ViewHiding should be called only if ViewShown was called.

The order of popuphiding and ViewHiding can't be guaranteed because we would likely have to call ViewHiding from one of the popuphiding handlers. The same goes for popuphidden and ViewHidden when they are called as a result of the panel being hidden, rather than the operation being canceled.

I'll file a bug to make the events more consistent.
Flags: needinfo?(paolo.mozmail)
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/085796417462
Part 1 - Fix tests for the activated page action panel. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/22c83b1d417d
Part 2 - Use an asynchronous API to open PanelMultiView panels. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8115e90ffa8
Part 3 - Prepare the main view just before opening the panel. r=Gijs
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27a1bfb7e792
Backed out 3 changesets for frequent leaks in AbstractThread, AnimationTimeline, Annotators, Array, AsyncFreeSnowWhite, ... on a CLOSED TREE
Hm, I've seen these leaks in earlier versions, but I thought they were fixed by setting the new properties to null on destruction.

Let's see if part 3 actually reintroduced them:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a65f4b93241a9d47dea5e71d9e241737c92c2ce
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f678015bb65ff37467e2252987a36bc1d725b2a0
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9abfd413e1d5
Part 1 - Fix tests for the activated page action panel. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2547ff9733
Part 2 - Use an asynchronous API to open PanelMultiView panels. r=Gijs
Comment on attachment 8947519 [details]
Bug 1434883 - Part 3 - Prepare the main view just before opening the panel.

The macOS leaks introduced by this patch are deterministic on try, but I couldn't reproduce them in local debug test runs. I've tried without success to fix a few likely causes by making the node references weak and cleaning up the transition on "popuphidden". Another strategy could be to reduce this to a single test failing by bisecting which tests run, but it would be quite time consuming, so I've landed the first two parts separately.

Unfortunately part 3 blocks the asynchronous description height workaround and other fixes, and I guess it's one of the reasons why we are still unable to land changes to this code in a number of cases.

Maybe it's a good time to look into de-XBLing the control, which we want to do anyways, so we can call the destructor deterministically and avoid any weird reference management issues.
Attachment #8947519 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9abfd413e1d5
https://hg.mozilla.org/mozilla-central/rev/3c2547ff9733
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
So are the bits from part 3 going to happen in a different bug? If so, which one? :-)
Flags: needinfo?(paolo.mozmail)
Blocks: 1437811
Filed bug 1437811, tentatively depending on bug 1437512.
Flags: needinfo?(paolo.mozmail)
Blocks: 1438507
Regressions: 1816693
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: