1128774 - Reader Mode fails hard on initial Gmail loading

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Go to gmail.com and log in.
2) Wait until the page loads.
3) If the page appears, go back to step 1 (you can stay logged in).

After a few tries, I end up on a blank page. When clicking the URL bar, no URL is filled in the awesomescreen. When clicking the tabs button, the tab title is blank. Adding some logging, I found that webView.URL is null when this happens.

I doubt this is specific to Gmail, but I haven't been able to reproduce it anywhere else yet.

Comment 1

[Brian Nicholson (:bnicholson)]

There are no errors in the Xcode console. I tried to debug this using the Safari Develop console, but the console just closes itself when this happens. So far, it's been a big pain to debug.

Through some elimination, it looks like it could be related to reader mode. I commented out the ReaderMode helper creation in BVC didCreateTab(), and the STR don't seem to work anymore. Thought it might be memory related, so I added logging to didReceiveMemoryWarning() in BVC, but it's not called.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I'll take a look at this in more detail. Assigning to myself since the Reader Mode hint seems to be strong.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The first thing I noticed is that GMail does a couple of redirects. It looks like it does these redirects by settings window.location, which is why I don't see redirect responses from the HTTP requests.

We run Readability every time we get a onShow event. But in this case, gmail also triggers that location change pretty much immediately. I would think that should not make a difference, but it seems it does.

Couple notes:

While this is happening we do not call into the page with webView.evaluateJavascript() so we can eliminate that.

Commenting out the calls to webkit.messageHandlers.readerModeMessageHandler.postMessage() makes no difference, so it also has nothing to do with messaging back to the native code.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/132

This patch adds an extra check to `checkReadability` to find out of the body of the document actually has child elements. If it has not then we can already say that the page is most likely an empty page used to redirect to another page. The redirection is certainly what GMail is doing.

Unfortunately I don't yet fully understand the underlying issue. My gut feeling says it is  a race condition in the WKWebView code where it executes a User Script while the page is suddenly being reloaded. I think it could be worthwhile to write an isolated test case so that we can submit this to Apple if this guess is correct.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8558626 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/132

Sadly, doesn't fix the issue as mentioned in the PR.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Pull request

Here's another attempt. Haven't been able to hit the bug with this applied.

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8558746 [details] [review]
Pull request

Nice one. Seems to work well.

Lets include this but also file a followup that we should optimize this later (if possible) because from a performance and memory perspective I don't think it is awesome to serialize the DOM and then parse it again.

One way to optimize this is to look at the failing DOM and make a decision to not call Readability() at all, earlier. I tried doing that by looking at the number of child nodes in the body but obviously that is not enough.

Comment 8

[Brian Nicholson (:bnicholson)]

Merged.

(In reply to Stefan Arentz [:st3fan] from comment #7)
> Lets include this but also file a followup that we should optimize this
> later (if possible) because from a performance and memory perspective I
> don't think it is awesome to serialize the DOM and then parse it again.

Yeah, it's hard to say what the perf impact of this is. We actually do this in Gecko too [1], though the parsing part happens in a web worker.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?rev=ad3867772421#245