Closed Bug 1161793 Opened 9 years ago Closed 9 years ago

Pocket: Race condition in onShow handling

Categories

(Firefox :: Pocket, defect, P1)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: nate+bugzilla, Assigned: jaws)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
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)
Assignee: nobody → jaws
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8602324 - Flags: review?(gijskruitbosch+bugs)
The patch in this bug also fixes bug 1161514 for me.
(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 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.
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 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.
Attachment #8602324 - Flags: review?(gijskruitbosch+bugs)
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).
Flags: qe-verify?
Flags: firefox-backlog+
Attached patch Patch v2Splinter Review
This is a less invasive patch that basically does the same thing as the previous patch but without making scary changes to panelUI.js.
Attachment #8602324 - Attachment is obsolete: true
Attachment #8602918 - Flags: review?(dolske)
Hi Jared, can you provide a point value.
Iteration: --- → 40.3 - 11 May
Flags: needinfo?(jaws)
Attachment #8602918 - Flags: review?(dolske) → review+
Points: --- → 5
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(jaws)
Blocks: Pocket
Blocks: 1162005
Blocks: 1162044
Priority: -- → P1
QA Contact: andrei.vaida
https://hg.mozilla.org/mozilla-central/rev/fb497081c35f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8602918 [details] [diff] [review]
Patch v2

[Triage Comment]

Required for 38.0.5 Pocket launch.
Attachment #8602918 - Flags: approval-mozilla-release+
Attachment #8602918 - Flags: approval-mozilla-aurora+
Depends on: 1163319
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1218977
You need to log in before you can comment on or make changes to this bug.