1259729 - Pocket button is intermittently missing its icon and panel content on startup

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot

[Note]:
- This is an intermittent issue.

[Affected versions]:
- 46.0b5-build2 (20160324011246)

[Affected platforms]:
- Windows 8 x64
- Windows 7 x86
- Mac OS X 10.10.5
- Ubuntu 14.04 x86

[Steps to reproduce]:
1. Launch Firefox with a new profile.
2. Enter Customize Mode via menu (≡) → Customize.
3. Move the new tab [+] button right before the back [←] button, from the navigation bar.
4. Restart the browser.
5. Check Pocket's button styling. Click the button.

[Expected result]:
- The changes are successfully applied.
- The Pocket button is displayed properly.

[Actual result]:
- The changes are successfully applied.
- The Pocket button is missing its icon and clicking it shows an empty panel, see the attached screenshot.

[Regression range]:
- This is an intermittent issue, I was unable to isolate precise steps or determine other affected/unaffected versions, apart from 46.0b5-build2. Either way, I'm setting the keywords -- perhaps someone might get luckier than I did.
- I suspect Bug 1245074 might have something to do with this, especially since it just landed on beta, but there's no way I can tell for sure using my current test environments.

[Additional notes]:
- This is reproducible with and without e10s.
- I was unable to replicate this issue with other add-on buttons, i.e. Hello.

[Tracking Requested - why for this release]:
- Common scenario affected by a noticeable UI and functional bug identified mid-beta.

Comment 1

[Andrei Vaida [:avaida]]

It's also worth mentioning that the following error message gets thrown by Pocket.jsm:66:0 for this action:
> window.pktUI is undefined

Comment 2

[Andrei Vaida [:avaida]]

The steps specified in Comment 0 are the ones most likely to reproduce the bug, as far as I've managed to investigate, but I've noticed that specific platforms, e.g. Windows 7 x86, are showing this bug easily while moving any button from the toolbar to any other location.

Still, as mentioned in Comment 0 [Additional notes], the only button affected by this is Pocket.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Shane this is probably for you! This may already be covered in uplifts waiting to land for beta 6, if so, you can dupe this bug.  But if not it sounds like something we should fix before release.

Comment 4

[Shane Caraveo (:mixedpuppy)]

So far I've been unable to reproduce this on either win7 or osx, however this build of beta does not include the patch from Bug 1245074 so it would not be a regression from that.  It would be good to know if this is reproducible on aurora as well.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Of course, I tried one last time after posting...and got the bug.  But no console output as mentioned.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: fix attaching pocket styles on startup

The only thing that I see that would cause this is that onWindowOpened is not being called, thus pocket styles and code are not getting set onto the window.

I suspect that CUI is calling onWindowOpened prior to the pocket addon being ready, I believe that is caused by a combination of:
 a) a slow startup thus the intermittent nature, but easier to cause in a VM
 b) the async call in the pocket bootstrap startup that checks if the old pocket addon is installed.  The async call may not return until after CUI starts up and calls onWindowOpened. 

I haven't been able to reproduce under aurora on win7 and cannot repro on osx at all.  

I've looked at the minor differences between beta and aurora and see no reason any of them would cause the styles to not get set.

The attached patch should fix it, but I need to clone the beta repo and build to verify.  That will take all day on my vm, wondering if we can find someone that can verify faster.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: fix attaching pocket styles on startup

on second thought, checking pktUI on the window may be better

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: fix attaching pocket styles on startup

Rather than building beta, I built an addon (with change in this patch) and installed that into beta.  I am unable to reproduce the problem at this point.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: pocket.xpi

addon used to test.  I unziped the addon from a beta install, applied patch and ziped a new xpi.  set xpinstall.signatures.required to false so I could install an unsigned addon.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8734875 [details] [diff] [review]
fix attaching pocket styles on startup

Not sure who is around today, looking for a reviewer, then I'll leave it with @lizzard whether this is something for uplift.

Comment 11

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

Comment on attachment 8734875 [details] [diff] [review]
fix attaching pocket styles on startup

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

rs=me, if this fixes it it's ok with me, seems safe. Do we have any tests that open the pocket panel and check the contents? This would probably reproduce on a debug build with --run-until-failure.

Comment 13

[Shane Caraveo (:mixedpuppy)]

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

Comment 14

[Shane Caraveo (:mixedpuppy)]

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

Comment 15

[Shane Caraveo (:mixedpuppy)]

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

Comment 16

[Shane Caraveo (:mixedpuppy)]

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

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: turn on pocket addon during the pocket test, some additional tests

Expanded on the test file in pocket and turned pocket on during this test.  This validates that elements are present when pocket is on (and removed when off).  It does not introduce tests on panel contents.  Given the nature of this particular bug I feel pretty confident that the failure case is covered.  Initial test runs on fx-team look good, but a bunch of orange on beta [that seem unrelated]

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8735576 [details] [diff] [review]
turn on pocket addon during the pocket test, some additional tests

add alternate reviewer

Comment 19

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

Comment on attachment 8735576 [details] [diff] [review]
turn on pocket addon during the pocket test, some additional tests

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> Expanded on the test file in pocket and turned pocket on during this test. 
> This validates that elements are present when pocket is on (and removed when
> off).  It does not introduce tests on panel contents.  Given the nature of
> this particular bug I feel pretty confident that the failure case is
> covered.

Well, can you test that that is the case? Like, the test is reasonable, but if it doesn't catch the issue as-is, do we really understand the issue?

::: browser/extensions/pocket/test/browser.ini
@@ +1,4 @@
> +[DEFAULT]
> +support-files =
> +  head.js
> +  test.html

Add support-files under the test to which they apply.

::: browser/extensions/pocket/test/browser_pocket_ui_check.js
@@ +1,3 @@
>  "use strict";
>  
> +function checkWindowProperties(has, l) {

maybe "isPresent" instead of "has", which is confusing in that ternary ?

@@ +1,5 @@
>  "use strict";
>  
> +function checkWindowProperties(has, l) {
> +  for (let name of l) {
> +    is(has, !!window.hasOwnProperty(name), "property " + name + (has ? " is":" is not") + " present");

Spaces around operators like : please.

@@ +14,2 @@
>  add_task(function*() {
> +  yield promisePocketEnabled();

please register a clean up handler to reset any of the prefs involved here.

::: browser/extensions/pocket/test/head.js
@@ +22,5 @@
> +  }, 100);
> +  var moveOn = function() { clearInterval(interval); nextTest(); };
> +}
> +
> +function promiseWaitForCondition(aConditionFn) {

Don't do this or the waitForCondition thing, just use BrowserTestUtils instead. In fact, it seems you don't use these at all anyway?

@@ +29,5 @@
> +  return deferred.promise;
> +}
> +
> +function promisePocketEnabled() {
> +  let deferred = Promise.defer();

Don't use Promise.defer, use new Promise() instead.

In this case, in the first if() you can just

return Promise.resolve(true);

For the remainder, you can use:

return new Promise(resolve => {
  // add listener, and from the listener, call resolve(true);
});

@@ +36,5 @@
> +    ok(true, "pocket was already enabled");
> +    deferred.resolve(true);
> +    return deferred.promise;
> +  }
> +  ok(true, "pocket is not enabled");

This should probably just be an info().

@@ +52,5 @@
> +  return deferred.promise;
> +}
> +
> +function promisePocketReset() {
> +  let deferred = Promise.defer();

Same note as before.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #19)
> Comment on attachment 8735576 [details] [diff] [review]
> turn on pocket addon during the pocket test, some additional tests
> 
> Review of attachment 8735576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> > Expanded on the test file in pocket and turned pocket on during this test. 
> > This validates that elements are present when pocket is on (and removed when
> > off).  It does not introduce tests on panel contents.  Given the nature of
> > this particular bug I feel pretty confident that the failure case is
> > covered.
> 
> Well, can you test that that is the case? Like, the test is reasonable, but
> if it doesn't catch the issue as-is, do we really understand the issue?

I changed the patch to work regardless of whether pocket is enabled on startup, enabled pocket in tests, and initially I was able to intermittently reproduce the error that way.  After repeated runs I am unable to (and see that onWindowOpened is called prior to the test all the time).

The essence of the problem is that the bootstrap startup wraps everything in an async to the addonmanager.  This creates a potential for CUI to initialize and call onWindowOpened prior to the pocket addon installing the CUI listener.  If that happens, then styles and window properties are not installed on the window, resulting in this bug.  Without the window properties (e.g. pktUI) set, the panel fails when opened resulting in the message in comment 1 and behavior as shown in the image from the reporter.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: turn on pocket addon during the pocket test, some additional tests

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: turn on pocket addon during the pocket test, some additional tests

uploading the correct patch would help.

Comment 23

[Shane Caraveo (:mixedpuppy)]

I'd been spinning wheels trying to figure out some oranges on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58194e57f9e0

But I see that a push without my patches has the same problems:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da8775314086&selectedJob=18834012

I'm going to assume now that it is not my changes causing those.

Comment 24

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

Comment on attachment 8736497 [details] [diff] [review]
turn on pocket addon during the pocket test, some additional tests

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

::: browser/extensions/pocket/test/browser_pocket_ui_check.js
@@ +15,5 @@
> +  let enabledOnStartup = yield promisePocketEnabled();
> +  registerCleanupFunction(() => {
> +    // Extra insurance that this is disabled again, but it should have been set
> +    // in promisePocketReset.
> +    Services.prefs.setBoolPref("extensions.pocket.enabled", enabledOnStartup);

Nit: just use Services.prefs.clearUserPref("extensions.pocket.enabled"); ?

::: browser/extensions/pocket/test/head.js
@@ +14,5 @@
> +  }
> +  info( "pocket is not enabled");
> +  return new Promise((resolve, reject) => {
> +    let listener = {
> +      onWidgetAfterCreation: function(widgetid) {

Nit: this is new code, let's use: onWidgetAfterCreation(widgetid) {

@@ +27,5 @@
> +    Services.prefs.setBoolPref("extensions.pocket.enabled", true);
> +  });
> +}
> +
> +function promisePocketDisalbed() {

Nit: Autocomplete is nice, but we should probably make sure that this is called "Disabled" rather than "Disalbed". :-)

@@ +59,5 @@
> +
> +function promisePocketReset() {
> +  if (enabledOnStartup) {
> +    info("reset is enabling pocket addon");
> +    return promisePocketEnabled();

Same, can't we use Services.prefs.clearUserPref("extensions.pocket.enabled"); ?

Comment 25

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #24)

> @@ +59,5 @@
> > +
> > +function promisePocketReset() {
> > +  if (enabledOnStartup) {
> > +    info("reset is enabling pocket addon");
> > +    return promisePocketEnabled();
> 
> Same, can't we use
> Services.prefs.clearUserPref("extensions.pocket.enabled"); ?

/testing/profiles/prefs_general.js are all user prefs (seems odd to me) and that is where we turn off pocket for testing.  clearUserPref would reset the pref which we dont want right now.  When we default pocket back on, we can change this to clearUserPref.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: turn on pocket addon during the pocket test, some additional tests

feedback from review comment addressed.

Comment 27

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/24134582d0ce49abdb224820c95e5ae5dae60174
Bug 1259729 turn on pocket during pocket test, r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/9b5113e833ff005d4845e88d8de1041985a8c5c7
Bug 1259729 fix attaching pocket styles on startup, r=jaws

Comment 28

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

Backed out for test failures in https://hg.mozilla.org/integration/fx-team/rev/543ba2e092d2

https://treeherder.mozilla.org/logviewer.html#?job_id=8365986&repo=fx-team

Comment 29

[Shane Caraveo (:mixedpuppy)]

Ok, the failure is due to bug 1261456.  I'm going to place all the support files under default, that gets built correctly and tests pass.

Comment 30

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/8e350c4b966f97902ab2d95cd9d70a88d1e70c68
Bug 1259729 turn on pocket during pocket test, r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/6f1d7279737d7e3ad4194947f81aeaad4e2c89a4
Bug 1259729 fix attaching pocket styles on startup, r=jaws

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e350c4b966f
https://hg.mozilla.org/mozilla-central/rev/6f1d7279737d

Comment 33

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8736862 [details] [diff] [review]
turn on pocket addon during the pocket test, some additional tests

Approval Request Comment
[Feature/regressing bug #]:pocket
[User impact if declined]:none
[Describe test coverage new/current, TreeHerder]:test only patch turns on basic tests
[Risks and why]: none for the test, no core code, but requires other patch in this bug to pass
[String/UUID change made/needed]:none

Comment 34

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8734875 [details] [diff] [review]
fix attaching pocket styles on startup

Approval Request Comment
[Feature/regressing bug #]:pocket
[User impact if declined]:intermittent failure to work on startup
[Describe test coverage new/current, TreeHerder]:new mochitest in test patch
[Risks and why]: low for aurora, higher for beta due to late stage, but beta is most affected
[String/UUID change made/needed]:none

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8734875 [details] [diff] [review]
fix attaching pocket styles on startup

Fix for pocket icons, ok to uplift for beta 8 and aurora.

Comment 36

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

https://hg.mozilla.org/releases/mozilla-beta/rev/a73f153c9e5c

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8736862 [details] [diff] [review]
turn on pocket addon during the pocket test, some additional tests

Tests for aurora.

Comment 38

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/359ec2194f1b
https://hg.mozilla.org/releases/mozilla-aurora/rev/adb277d1c5a7

Comment 40

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 46b8 Win 7, Ubuntu 14.04, OS X 10.10.5.

Comment 41

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 48.0a2 (2016-05-08), 47b3 Win 7.