Closed Bug 1152412 Opened 5 years ago Closed 5 years ago

Reading list items that are images get stuck in "loading" state

Categories

(Toolkit :: Reader Mode, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 + fixed
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: Dolske, Assigned: Margaret)

Details

Attachments

(1 file, 1 obsolete file)

1) Add an image file to reader list (may I suggest http://i.imgur.com/GleAY3f.jpg)
2) Open a random article in reader mode
3) Click the image in the reader list sidebar (added in #1)

I get an empty page with "Loading..." showing, which stays that way. We should load the image (and presumably exit reader mode).

This does work correctly if the item I add to the reading list is, instead of an image, a page that fails to work in reader mode. (ie, one that normally results in "Failed to load article from page" when entering reader mode).
I see in the console:
TypeError: doc is null ReaderMode.jsm:136:12

Looks like our XHR logic makes assumptions about getting a document:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#136
Assignee: nobody → margaret.leibovic
/r/6945 - Bug 1152412 - Handle errors downloading and parsing documents for reader view. r=bnicholson

Pull down this commit:

hg pull -r 3bb7f0207971e48445ad46ddc28d2855d8ba1c3f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8591103 - Flags: review?(bnicholson)
(In reply to :Margaret Leibovic from comment #1)
> I see in the console:
> TypeError: doc is null ReaderMode.jsm:136:12
> 
> Looks like our XHR logic makes assumptions about getting a document:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> ReaderMode.jsm#136

This actually exposed a bigger problem - the promise we return from downloadAndParseDocument can be rejected, but we aren't currently handling this.

I reproduced this bug on Android and verified this patch also fixes the Android case.
Comment on attachment 8591103 [details]
MozReview Request: bz://1152412/margaret

https://reviewboard.mozilla.org/r/6943/#review5823

Ship It!
Attachment #8591103 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/6f2c078e1be4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Flags: qe-verify+
QA Contact: andrei.vaida
This is verified fixed on Nightly 40.0a1 (2015-04-15) using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5, but the following error message is thrown in the Browser Console each time you click the image from the sidebar:
> Error downloading and parsing document: Reader mode XHR didn't return a document
in ReaderParent.jsm:218:0 / ReaderParent.jsm:201:0. Margaret, what's your take on this?
Status: RESOLVED → VERIFIED
Flags: needinfo?(margaret.leibovic)
(In reply to Andrei Vaida, QA [:avaida] from comment #7)
> This is verified fixed on Nightly 40.0a1 (2015-04-15) using Windows 7 (x64),
> Ubuntu 14.04 (x64) and Mac OS X 10.9.5, but the following error message is
> thrown in the Browser Console each time you click the image from the sidebar:
> > Error downloading and parsing document: Reader mode XHR didn't return a document
> in ReaderParent.jsm:218:0 / ReaderParent.jsm:201:0. Margaret, what's your
> take on this?

That's expected, but I think it's good to keep the error message in the log so that we know why we couldn't open the image in reader mode.
Flags: needinfo?(margaret.leibovic)
Are you planning on uplifting this to aurora, beta, and release?
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8591103 [details]
MozReview Request: bz://1152412/margaret

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Are you planning on uplifting this to aurora, beta, and release?

Good catch! I somehow missed this one.

Approval Request Comment
[Feature/regressing bug #]: Reader view
[User impact if declined]: JS errors result in busted reader view state
[Describe test coverage new/current, TreeHerder]: landed on Nightly
[Risks and why]: low-risk, adds logic to catch errors and handle them more gracefully
[String/UUID change made/needed]: none
Flags: needinfo?(margaret.leibovic)
Attachment #8591103 - Flags: approval-mozilla-release?
Attachment #8591103 - Flags: approval-mozilla-beta?
Comment on attachment 8591103 [details]
MozReview Request: bz://1152412/margaret

Safe fix that has been on 40 for a month and is also verified. Approved for 38.0.5 and 39.
Attachment #8591103 - Flags: approval-mozilla-release?
Attachment #8591103 - Flags: approval-mozilla-release+
Attachment #8591103 - Flags: approval-mozilla-beta?
Attachment #8591103 - Flags: approval-mozilla-beta+
Attachment #8591103 - Attachment is obsolete: true
Attachment #8619988 - Flags: review+
You need to log in before you can comment on or make changes to this bug.