Closed Bug 1261375 Opened 4 years ago Closed 4 years ago
'Pocket Saving' spins forever when moved to the menu panel and trying to save the New Tab Page
[Affected versions]: - 46b6 [Affected platforms]: - all [Steps to reproduce]: 1. Start FX with a new profile 2. Enter 'Customize' mode and move the Pocket icon to the menu panel 3. Sign in to Pocket 4. Open the New Tab Page 5. Click on the Pocket button in the menu panel [Expected result]: - "Only links can be saved" error message should be displayed [Actual result]: - 'Saving' spins forever [Regression range]: - reproduced on FX 45, 48.0a1 (2016-03-31)
since this affects 45, I'm removing regression keyword.
Shane, Mike, double checking this doesn't block pocket as system addon shipping in 46. I'm assuming it doesn't and wontfixing for 46. Sounds like it may never have worked as expected. We should probably fix it though.
[Tracking Requested - why for this release]: no, but lets track for 47
Tracked for 47, I hope we can uplift a fix soon, after 47.0b6 the uplift bar will be pretty high and only recent regressions, sec and stability issues will make the cut.
Hi Shane, please see comment 4. I hope we have a fix ready in the next week or so otherwise I will have to resolve this as a wontfix in the light of other more severe 47 tracked issues. :(
fix is in progress. https://reviewboard.mozilla.org/r/49337/
Review commit: https://reviewboard.mozilla.org/r/49337/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49337/
Attachment #8746258 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8746258 [details] MozReview Request: Bug 1261375 fix load handling in pocket panel, r?gijs https://reviewboard.mozilla.org/r/49337/#review46275
Attachment #8746258 - Flags: review?(gijskruitbosch+bugs) → review+
At some point it might be good to have tests for some of this stuff...
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
(also, do we know why we thought the readyState stuff was necessary? I tested a bit and couldn't reproduce any issues with the patch, so this seems OK, but it's a little unsettling...)
(In reply to Shane Caraveo (:mixedpuppy) from comment #6) > fix is in progress. https://reviewboard.mozilla.org/r/49337/ Thanks! Really appreciate the prompt follow up.
(In reply to :Gijs Kruitbosch from comment #10) > (also, do we know why we thought the readyState stuff was necessary? I > tested a bit and couldn't reproduce any issues with the patch, so this seems > OK, but it's a little unsettling...) readyState is a pattern I use often with iframes in panels to make sure the iframe load doesn't beat onshown. In any case, the new patch I just pushed keeps this and just checks that the uri is not about:blank. It always seems to be about:blank, but this would catch an edge case [wouldn't surprise me if there is one].
Comment on attachment 8746258 [details] MozReview Request: Bug 1261375 fix load handling in pocket panel, r?gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49337/diff/1-2/
ping for updated patch review
This also looks good to me. Sorry, I did not get flagged for review again because mozreview assumed my review would carry (there isn't currently a way for you to fix this as the patch submitter. See e.g. bug 1247772, bug 1247069, ...)
https://hg.mozilla.org/integration/fx-team/rev/ca1a70889b2bc2009bb18b831598fec26a485919 Bug 1261375 fix load handling in pocket panel, r=gijs
Comment on attachment 8746258 [details] MozReview Request: Bug 1261375 fix load handling in pocket panel, r?gijs Approval Request Comment [Feature/regressing bug #]:pocket [User impact if declined]:In some instances pocket will fail (timing related) [Describe test coverage new/current, TreeHerder]:on central [Risks and why]: low, this just adds a check that the loaded document is not about:blank [String/UUID change made/needed]: none
Verified fixed FX 49.0a1 (2016-05-08) Win 7, Ubuntu 14.04, OS X 10.10.5
Comment on attachment 8746258 [details] MozReview Request: Bug 1261375 fix load handling in pocket panel, r?gijs Recent regression in Pocket. Fix was verified on Nightly, Aurora48+, Beta47+
Setting the flag for verification on 47.0b4.
Verified fixed FX 47b8, 48.0a2 (2016-05-26) Win 7.
You need to log in before you can comment on or make changes to this bug.