1161793 - Pocket: Race condition in onShow handling

Status: VERIFIED FIXED

(Firefox :: Pocket, defect, P1)

Description

[Nate Weiner]

There is a race condition occurring with the panel messaging (panel <=> chrome) that is causing a number of issues, primarily an infinite spinner showing on the save dialog or panels not loading.

This is the root cause of the following bugs:
- https://github.com/Pocket/Firefox/issues/15
- https://github.com/Pocket/Firefox/issues/17
- The missing error state described in: https://bugzilla.mozilla.org/show_bug.cgi?id=1161654#c4

--

The problem:

The cause of the bug is an expected handling of the onShow callback of the showPanel method.

In the add-on code, when the button is clicked the following is called:
1. pktUI.pocketButtonOnCommand which goes all the way down to pktUI.saveAndShowConfirmation
2. This calls pktUI.showPanel and passes a callback onShow that is expected to be called after the panel shows.
3. The panel is then loaded and the javascript inside of the panel initializes
4. The panel then triggers it's onpopupshown shown event which calls pktUI.pocketPanelDidShow, which in turns calls the onShow callback
5. This onShow callback then sends a message to the panel and continues

However, the code in native integration is doing something slightly different and not using the native onpopupshown event callback. When the Pocket button is clicked:
1. Calls onPanelViewShowing in Pocket.jsm
2. This method then in one line after the other calls: pktUI.pocketButtonOnCommand and pktUI.pocketPanelDidShow, so the following occurs:
3. pktUI.pocketButtonOnCommand which goes all the way down to pktUI.saveAndShowConfirmation
4. This calls pktUI.showPanel and passes a callback onShow that is expected to be called after the panel shows.
5. [race condition occurs here]
6. pktUI.pocketPanelDidShow is called immediately before the panel can potentially load and the content inside of it initializes. 
7. This immediately calls the onShow callback which tries to send a message to the panel, but the panel hasn't loaded yet so the message is never received.

As a result, the panel ends up in a state that will never complete.

-- 

The solution:

A) The ideal solution is that pktUI.pocketPanelDidShow gets called when the panel shows (using the onpopupshown event). This will mirror what the add-on code is doing and will make it easier for us to maintain consistency there.

B) If there is a problem with doing that, we can update our messaging framework to have the panel send a message up to the chrome to say "I've loaded" and then use that in place of the onpopupshown event. This will work, but it's possible this introduces a flash or delay after the panel loads before content pops in.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Moving the width/height to the showPanel function call more closely reflects the code in the Pocket repo at https://github.com/Pocket/Firefox/blob/develop/chrome/content/main.js (private/closed repo)

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The patch in this bug also fixes bug 1161514 for me.

Comment 3

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Created attachment 8602324 [details] [diff] [review]

https://hg.mozilla.org/try/rev/133f5dc5ddaf

Comment 4

[Justin Dolske [:Dolske]]

Err. Rather: https://treeherder.mozilla.org/#/jobs?repo=try&revision=133f5dc5ddaf

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8602324 [details] [diff] [review]
Patch

Review of attachment 8602324 [details] [diff] [review]:
-----------------------------------------------------------------

It's been too long since I've worked on panelUI.js, and I don't know Pocket code, so the ordering and nuances are a little lost on me.

What I do know, is that the defaultPrevented thing was added in https://hg.mozilla.org/mozilla-central/rev/788d4dc9aaca, and the Feed button is the only button in CustomizableWidgets.jsm to use it, so before landing, we should hammer on the Feed button to make sure it still does what it's supposed to.

Otherwise (along with the dangling panel thing I found), this looks reasonable to me for the panelUI.js stuff.

::: browser/components/customizableui/content/panelUI.js
@@ +361,5 @@
> +      let evt = document.createEvent("CustomEvent");
> +      evt.initCustomEvent("ViewShowing", true, true, viewNode);
> +      viewNode.dispatchEvent(evt);
> +      if (evt.defaultPrevented) {
> +        return;

Drive-by - you're going to need to remove the panel here, otherwise we'll have one hanging around, appended to the nav-bar that never got removed (since it never opened and then closed).

So before you return, you should remove the panel from the DOM.

Comment 6

[:Gijs (out for now; he/him)]

I will have time for an indepth review later today, but I'm 99% sure that we have code (and/or addons) that depends on ViewShowing callbacks being called *before* the popup shows. Changing this on branch seems like a not great idea. If anything, maybe we should have a new callback like AfterViewShown or whatever?

Comment 7

[:Gijs (out for now; he/him)]

Comment on attachment 8602324 [details] [diff] [review]
Patch

Review of attachment 8602324 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at this again, I guess it's slightly less invasive than I thought, but I don't understand why you need both the popupshown listener and the changes in panelUI.js. It seems like just the viewID setting and the Pocket.jsm / main.js changes should be sufficient.

Comment 8

[Justin Dolske [:Dolske]]

Would an ugly setTimeout(0) hack (in the ViewShowing listener) work? Pocket doesn't need to use a (synchronous) preventDefault(), so a setTimeout(0) should be enough to let the event dispatch return, panel be created, and things carry on. But a AfterViewShown event would be fine too -- both make me more comfortable in that we're not changing panelUI.js in ways that might cause compat issues (with none of the normal soak time to catch bugs).

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This is a less invasive patch that basically does the same thing as the previous patch but without making scary changes to panelUI.js.

Comment 10

[Marco Mucci [:MarcoM]]

Hi Jared, can you provide a point value.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/fb497081c35f

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb497081c35f

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 8602918 [details] [diff] [review]
Patch v2

[Triage Comment]

Required for 38.0.5 Pocket launch.

Comment 15

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5334003ba7b8
https://hg.mozilla.org/releases/mozilla-release/rev/18bf7b4baaac

Comment 16

[Ada [:adalucinet]]

No issues regarding Pocket panel (including the ones mentioned in comment 0) encountered with Firefox 38.0.5 beta 3 (Build ID: 20150518141916), across the following platforms: Windows 7 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.9.5.