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

Categories

(Firefox :: Pocket, defect)

46 Branch
defect
Not set

Tracking

()

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

People

(Reporter: pauly, Assigned: mixedpuppy)

References

Details

Attachments

(2 files)

[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)
Keywords: regression
No longer blocks: 1215694
since this affects 45, I'm removing regression keyword.
Keywords: regression
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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mconnor)
[Tracking Requested - why for this release]:

no, but lets track for 47
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mconnor)
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. :(
Flags: needinfo?(mixedpuppy)
fix is in progress.  https://reviewboard.mozilla.org/r/49337/
Flags: needinfo?(mixedpuppy)
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
Flags: needinfo?(gijskruitbosch+bugs)
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, ...)
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/ca1a70889b2b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
Attachment #8746258 - Flags: approval-mozilla-beta?
Attachment #8746258 - Flags: approval-mozilla-aurora?
Verified fixed FX 49.0a1 (2016-05-08) Win 7, Ubuntu 14.04, OS X 10.10.5
Status: RESOLVED → VERIFIED
Blocks: 1271238
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+
Attachment #8746258 - Flags: approval-mozilla-beta?
Attachment #8746258 - Flags: approval-mozilla-beta+
Attachment #8746258 - Flags: approval-mozilla-aurora?
Attachment #8746258 - Flags: approval-mozilla-aurora+
Setting the flag for verification on 47.0b4.
Flags: qe-verify+
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.