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

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Paul Silaghi, QA [:pauly]]

[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

Comment 1

[Shane Caraveo (:mixedpuppy)]

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).

Comment 2

[Paul Silaghi, QA [:pauly]]

(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.

Comment 3

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

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?

Comment 4

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

I could still take a patch for beta 11, which will go to build tomorrow.

Comment 5

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

mconnor do you have an opinion here? blocker or not in your judgement?

Comment 6

[Shane Caraveo (:mixedpuppy)]

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.

Comment 7

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

Wontfix for 46 from comment 6.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: 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.

Comment 9

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

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?

Comment 10

[Shane Caraveo (:mixedpuppy)]

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].

Comment 11

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

(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!

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: v2 fix intermittent missing menu items

Chatted with Gijs and learned a new CUI trick.  Who says old dogs cant learn new tricks?

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/86e0ea42f55c34fc90dd9e32439783a9929f28f4
Bug 1263599 fix missing menu items in pocket, r=Gijs

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86e0ea42f55c

Comment 15

[Shane Caraveo (:mixedpuppy)]

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

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8745053 [details] [diff] [review]
v2 fix intermittent missing menu items

Recent regression, Aurora48+, Beta47+

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Paul, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 49.0a1 (2016-04-28) Win 7, Ubuntu 14.04, OS X 10.9.5.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Please hold off on uplift until bug 1268508 is resolved.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 21

[Shane Caraveo (:mixedpuppy)]

We can move forward with uplift.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8745053 [details] [diff] [review]
v2 fix intermittent missing menu items

The perf issue wasn't a blocker to uplift this.

Comment 23

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/355df6a7a69e

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/5191eb887b1e

Comment 25

[Andrei Vaida [:avaida]]

Setting the flag back for verification on 47.0b2 and 48.0a2.

Comment 26

[Paul Silaghi, QA [:pauly]]

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