1364056 - Reader view is broken some of the time due to attempting to use dead 'document' references to get a URL

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Ludovic Hirlimann [:Usul]]

For the last 2 days I've been unable to use the reader view mode. Each time I try (different websites) I get unable to fetch the article from the page (something similar as I'm using the french locale)

Comment 1

[Pascal Chevrel:pascalc]

Confirmed with today's build

Comment 2

[Stoyan Dimitrov [:stoyan]]

Probably locale dependant. Works for me. Android in English with latest Nightly language set to Bulgaria.

Comment 3

[Marcia Knous [:marcia]]

[Tracking Requested - why for this release]: Recent regression in Reader view

I see the issue after I updated to the latest nightly on en-US build on my Nexus 6.

Any article from the nytimes.com and slate.com generated a message "Failed to load article from page."

Using 20170511133403

Comment 4

[Sorina Florean [:sflorean]]

I was able to reproduce this with Asus ZenPad 8(Android 6.0.1) on articles from nytimes.com.

 Regression window:
Last good build: 05-09
First bad build: 05-10
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b21b974d60d3075ae24f6fb1bae75d0f122f28fc&tochange=ce2218406119c36a551e3faea4e192186ee46cc5

Comment 5

[Marcia Knous [:marcia]]

Thanks Sorina for the regression window. There are a lot of changes in there - ni on Wesley to find someone who can take a closer look at the window.

Comment 6

[Pascal Chevrel:pascalc]

Although this is reported for Firefox for Android, this is also impacting Firefox for Desktop.

Here is an example of an article triggering the bug
https://blog.nightly.mozilla.org/2017/04/25/guest-post-india-uses-firefox-nightly-kick-off-on-may-1-2017/

Launching Nightly in the console, we have a en exception thrown in the console when you click on the reader mode icon:
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: can't access dead object
Full stack: this.ReaderMode._readerParse<@resource://gre/modules/ReaderMode.jsm:478:5
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
get _worker/worker.onmessage@resource://gre/modules/PromiseWorker.jsm:231:9
EventHandlerNonNull*get _worker@resource://gre/modules/PromiseWorker.jsm:217:5
postMessage@resource://gre/modules/PromiseWorker.jsm:291:9
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
post@resource://gre/modules/PromiseWorker.jsm:263:12
this.ReaderMode._readerParse<@resource://gre/modules/ReaderMode.jsm:463:23
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
this.ReaderMode.parseDocument<@resource://gre/modules/ReaderMode.jsm:234:18
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
receiveMessage@chrome://browser/content/tab-content.js:277:34

*************************
Note that I can reproduce that with both en-US and fr builds on Desktop so that doesn't seem to be locale dependent

Comment 7

[Pascal Chevrel:pascalc]

Looking at all the files mentioned in the exception, only ReaderMode.jsm was modified between the May 9 and 10:
https://hg.mozilla.org/mozilla-central/rev/9a806ddbcfc6

Maybe a clue for investigation

Comment 8

[Marcia Knous [:marcia]]

Tracking 55+.

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/annotate/9a806ddbcfc6/toolkit/components/reader/ReaderMode.jsm#l478 is the line of the error here. `doc` must be dead by then. We should save the URL bit we want before doing the parse in the worker. I don't have cycles to write a patch before Monday, but if someone else can do it I can probably rubberstamp.

Still pretty confused how this is hitting people in practice despite automated tests working, and manual testing when I wrote these patches...

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1364056 - don't try to use `doc` reference when it might be dead,

Review commit: https://reviewboard.mozilla.org/r/139426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139426/

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8867885 [details]
Bug 1364056 - don't try to use `doc` reference when it might be dead,

https://reviewboard.mozilla.org/r/139426/#review143096

Looks like my patch :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79c28571253b
don't try to use `doc` reference when it might be dead, r=baku

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79c28571253b

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3563f72474d3