Closed Bug 1263599 Opened 8 years ago Closed 8 years ago

"View Pocket List" option from the bookmarks menu gone after restart

Categories

(Firefox :: Pocket, defect)

46 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox45 --- unaffected
firefox46 + wontfix
firefox47 + verified
firefox48 + verified
firefox49 --- verified

People

(Reporter: pauly, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

[Affected versions]:
- 46b9, 48.0a1 (2016-04-10)

[Affected platforms]:
- all

[Steps to reproduce]:
1. Start FX with a new profile
2. Check the "View Pocket list" option on the bookmarks menu in the toolbar
3. Restart Firefox
4. Open the bookmarks menu again

[Expected result]:
- "View Pocket List" option displayed

[Actual result]:
- "View Pocket List" option is gone after restart

[Regression range]: 
- regressed by bug 1259729
Keywords: regression
Can you test a little further?  Does the menu remain hidden on more than one restart?  I've verified the  menu is missing on the first restart, but after that it is always visible. (strange).
Flags: needinfo?(paul.silaghi)
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> I've verified the  menu is missing on the first restart, but after
> that it is always visible. (strange).
Confirmed.

I found another way to reproduce the problem:
1. Start FX with a new profile
2. Right click on the bookmarks button/Move to menu
3. Restart Firefox
4. Right click on the bookmarks button/Move to toolbar
5. Open the bookmarks menu

[Actual result]:
- "View Pocket List" option is gone

Note: This is also reproducible only once. After restart, the option is back. If you want to reproduce again, the steps must executed again.
Flags: needinfo?(paul.silaghi)
We should fix this for 46. I'm not sure if it should block release, but it seems pretty bad since it affects how the user will first see this feature. Shane, what do you think?
Flags: needinfo?(mixedpuppy)
I could still take a patch for beta 11, which will go to build tomorrow.
mconnor do you have an opinion here? blocker or not in your judgement?
Flags: needinfo?(mconnor)
I looked at this yesterday.  I can reproduce on beta, but not on a beta build I create, nor on deved or nightly.  The diff between nightly pocket and beta pocket are very minimal with no obvious difference that would cause a problem like this. essentially, it would take more time than we have, so we'll have to live with this one for now.
Flags: needinfo?(mixedpuppy)
Wontfix for 46 from comment 6.
This is caused because the pocket addon startup makes an async call prior to initializing. Most of the time it beats CUI onWindowOpened, sometimes it didn't and some items ended up missing from the UI.  Tests didn't catch it because it is a startup issue only.  Moving the init out of the async call fixes this.  This has been the cause of a couple problems in the past.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mconnor)
Attachment #8744414 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8744414 [details] [diff] [review]
fix intermittent missing menu items

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> Created attachment 8744414 [details] [diff] [review]
> fix intermittent missing menu items
> 
> This is caused because the pocket addon startup makes an async call prior to
> initializing. Most of the time it beats CUI onWindowOpened, sometimes it
> didn't and some items ended up missing from the UI.  Tests didn't catch it
> because it is a startup issue only.  Moving the init out of the async call
> fixes this.  This has been the cause of a couple problems in the past.

So, I don't like being annoying, but I'm also really troubled by this. Even with the code change you just made, there's no reason that you'll keep beating onWindowOpened, especially not if you're e.g. runtime-disabled.

You already add yourself to all the windows CUI knows about in the initialization code, so this explanation also doesn't really make sense to me. If your code runs after onWindowOpened, the window should be in the list that you then add yourself to in the relevant loop over open browser windows. Why is the menu (specifically - is the toolbarbutton there in these cases?) not present in these cases? I feel like I don't really understand why the current system doesn't work.

Completely orthogonally to that: have we tested that setting userDisabled on the old pocket add-on actually works? Is it even restartless?
Flags: needinfo?(mixedpuppy)
Attachment #8744414 - Flags: review?(gijskruitbosch+bugs)
In PocketOverlay.updateWindow, called by onWindowOpened, we get elements we want to insert before.  Because the async call wrapping the bootstrap startup, sometimes CUI calls onWindowOpened prior to the addon startup, sometimes updateWindow is called prior to the window being ready (and the elements we're looking for in updateWindow are not there), sometimes not (and things work).  

By moving the startup init out of the async call, the bootstrap startup always happens prior to CUI calling onWindowOpened and everything works consistently.  

The only thing better I can do is wait for browser-delayed-startup-finished notifications rather than relying on CUI onWindowOpened call, but I still need to move the bootstrap startup out of the async call.

If the addon is runtime enabled it makes the call itself to insert into the windows in PocketOverlay.startup.

I can change this to disable the system addon rather than the old one.  Worst case scenario, the button will appear/disappear once for some people who have the old addon enabled [first time startup with new addon].
Flags: needinfo?(mixedpuppy) → needinfo?(gijskruitbosch+bugs)
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> In PocketOverlay.updateWindow, called by onWindowOpened, we get elements we
> want to insert before.  Because the async call wrapping the bootstrap
> startup, sometimes CUI calls onWindowOpened prior to the addon startup,
> sometimes updateWindow is called prior to the window being ready (and the
> elements we're looking for in updateWindow are not there), sometimes not
> (and things work).

It seems like you should be able to rely on menu_bookmarkThisPage being there. It's in an included file in browser.xul, AFAICt. If it isn't there, presumably the window has not finished loading, so we should wait for the DOMContentLoaded event to happen. Not delayed-startup; just DCL (which happens earlier).

> The only thing better I can do is wait for browser-delayed-startup-finished
> notifications rather than relying on CUI onWindowOpened call, but I still
> need to move the bootstrap startup out of the async call.

If we wait for DOMContentLoaded, all the elements you need should be there, right? Why do we need to move it out of the async call? It feels like there's something I'm still not getting...

> I can change this to disable the system addon rather than the old one. 
> Worst case scenario, the button will appear/disappear once for some people
> who have the old addon enabled [first time startup with new addon].

That does seem less regression-prone, but either way I feel like I'm still in the dark about why we need to do that - sorry if I'm being dim!
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mixedpuppy)
Chatted with Gijs and learned a new CUI trick.  Who says old dogs cant learn new tricks?
Attachment #8744414 - Attachment is obsolete: true
Flags: needinfo?(mixedpuppy)
Attachment #8745053 - Flags: review?(gijskruitbosch+bugs)
Attachment #8745053 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/86e0ea42f55c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Target Milestone: Firefox 48 → Firefox 49
Comment on attachment 8745053 [details] [diff] [review]
v2 fix intermittent missing menu items

Approval Request Comment
[Feature/regressing bug #]:pocket
[User impact if declined]:missing "view pocket list" menu entries under bookmarks button/menu
[Describe test coverage new/current, TreeHerder]:we have a mochitest to check these elements, but this is an intermittent startup issue (timing related), fixed by relying on CUI to tell us what windows it already knows about.
[Risks and why]: low, minimal change
[String/UUID change made/needed]:none
Attachment #8745053 - Flags: approval-mozilla-beta?
Attachment #8745053 - Flags: approval-mozilla-aurora?
Comment on attachment 8745053 [details] [diff] [review]
v2 fix intermittent missing menu items

Recent regression, Aurora48+, Beta47+
Attachment #8745053 - Flags: approval-mozilla-beta?
Attachment #8745053 - Flags: approval-mozilla-beta+
Attachment #8745053 - Flags: approval-mozilla-aurora?
Attachment #8745053 - Flags: approval-mozilla-aurora+
Hi Paul, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(paul.silaghi)
Verified fixed FX 49.0a1 (2016-04-28) Win 7, Ubuntu 14.04, OS X 10.9.5.
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Please hold off on uplift until bug 1268508 is resolved.
Comment on attachment 8745053 [details] [diff] [review]
v2 fix intermittent missing menu items

Reverting flags to "?" based on last comment. I will review this one again next week and + if the other regression has been fixed.
Attachment #8745053 - Flags: approval-mozilla-beta?
Attachment #8745053 - Flags: approval-mozilla-beta+
Attachment #8745053 - Flags: approval-mozilla-aurora?
Attachment #8745053 - Flags: approval-mozilla-aurora+
We can move forward with uplift.
Flags: needinfo?(rkothari)
Comment on attachment 8745053 [details] [diff] [review]
v2 fix intermittent missing menu items

The perf issue wasn't a blocker to uplift this.
Flags: needinfo?(rkothari)
Attachment #8745053 - Flags: approval-mozilla-beta?
Attachment #8745053 - Flags: approval-mozilla-beta+
Attachment #8745053 - Flags: approval-mozilla-aurora?
Attachment #8745053 - Flags: approval-mozilla-aurora+
Setting the flag back for verification on 47.0b2 and 48.0a2.
Flags: qe-verify+
Verified fixed FX 48.0a2 (2016-05-08), 47b3 Win 7.
You need to log in before you can comment on or make changes to this bug.