Closed Bug 1259729 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect, major)

46 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox46 + verified
firefox47 --- verified
firefox48 --- verified

People

(Reporter: avaida, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(4 files, 5 obsolete files)

Attached image 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.
Severity: normal → major
It's also worth mentioning that the following error message gets thrown by Pocket.jsm:66:0 for this action:
> window.pktUI is undefined
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
Of course, I tried one last time after posting...and got the bug.  But no console output as mentioned.
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.
on second thought, checking pktUI on the window may be better
Attachment #8734865 - Attachment is obsolete: true
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.
Attachment #8734867 - Attachment is obsolete: true
Attached file 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 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.
Flags: needinfo?(lhenry)
Attachment #8734875 - Flags: review?(jaws)
Attachment #8734875 - Flags: review?(dolske)
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.
Attachment #8734875 - Flags: review?(jaws)
Attachment #8734875 - Flags: review?(dolske)
Attachment #8734875 - Flags: review+
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Duplicate of this bug: 1259376
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]
Attachment #8735576 - Flags: review?(jaws)
Comment on attachment 8735576 [details] [diff] [review]
turn on pocket addon during the pocket test, some additional tests

add alternate reviewer
Attachment #8735576 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8735576 - Flags: review?(jaws)
Attachment #8735576 - Flags: review?(gijskruitbosch+bugs)
Attachment #8735576 - Flags: feedback+
(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.
Attachment #8735576 - Attachment is obsolete: true
Attachment #8736495 - Flags: review?(gijskruitbosch+bugs)
uploading the correct patch would help.
Attachment #8736495 - Attachment is obsolete: true
Attachment #8736495 - Flags: review?(gijskruitbosch+bugs)
Attachment #8736497 - Flags: review?(gijskruitbosch+bugs)
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 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"); ?
Attachment #8736497 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
feedback from review comment addressed.
Attachment #8736497 - Attachment is obsolete: true
Attachment #8736862 - Flags: review+
See Also: → 1261374
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.
Flags: needinfo?(mixedpuppy)
Duplicate of this bug: 1261516
Summary: Pocket button is missing its icon and panel content after customizing the toolbar → Pocket button is intermittently missing its icon and panel content on startup
https://hg.mozilla.org/mozilla-central/rev/8e350c4b966f
https://hg.mozilla.org/mozilla-central/rev/6f1d7279737d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
Attachment #8736862 - Flags: approval-mozilla-aurora?
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
Attachment #8734875 - Flags: approval-mozilla-beta?
Attachment #8734875 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(lhenry)
Attachment #8734875 - Flags: approval-mozilla-beta?
Attachment #8734875 - Flags: approval-mozilla-beta+
Attachment #8734875 - Flags: approval-mozilla-aurora?
Attachment #8734875 - Flags: approval-mozilla-aurora+
Comment on attachment 8736862 [details] [diff] [review]
turn on pocket addon during the pocket test, some additional tests

Tests for aurora.
Attachment #8736862 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Duplicate of this bug: 1261374
Verified fixed FX 46b8 Win 7, Ubuntu 14.04, OS X 10.10.5.
Depends on: 1263599
Verified fixed FX 48.0a2 (2016-05-08), 47b3 Win 7.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.