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

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Justin Dolske [:Dolske]]

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

Comment 1

[:Margaret Leibovic]

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

Comment 2

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1152412/margaret

/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/

Comment 3

[:Margaret Leibovic]

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

[Brian Nicholson (:bnicholson)]

Comment on attachment 8591103 [details]
MozReview Request: bz://1152412/margaret

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

Ship It!

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6f2c078e1be4

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f2c078e1be4

Comment 7

[Andrei Vaida [:avaida]]

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?

Comment 8

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

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Are you planning on uplifting this to aurora, beta, and release?

Comment 10

[: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

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 12

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

https://hg.mozilla.org/releases/mozilla-release/rev/964442785c00

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1291a13c1fc5

Comment 15

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1152412 - Handle errors downloading and parsing documents for reader view. r=bnicholson