972723 - Reading list icon persists when aborting page load and going back to about:home

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Fred Wenzel [:wenzel]]

Version: Firefox for Android nightly
2014-02-13

STR:
- in about:home, history pane choose a page
- before it fully loads(!), press the Android "back" button
- note the "reading list" icon appears in the tool bar, despite you being back in about:home
- press the reading list icon and note it shows an error.

Expected:
- the reading list icon should not show up on about:home, even in this scenario.

Comment 1

[:Margaret Leibovic]

This sounds like our tab state is getting out of sync the page actions UI. We're likely updating the page actions UI before the tab state has reset itself.

Maybe we need to send a tab id along with the PageActions:Add message:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1762

Wes, what do you think?

Comment 2

[Wesley Johnston (:wesj)]

My original API for these was a lot more complex, but we decided to leave management of this up to the implementor. Glancing through browser.js, there are a lot of entry points into Reader.updatePageAction. I think we want to limit those...

Comment 3

[Gokul Caushik]

I would like to take this bug, is anyone mentoring this? 
Note:
I was able to reproduce this bug on my Nexus 7. 
I did a Fennec build on my computer so I have the source already.

Comment 4

[Aaron Train [:aaronmt]]

Hi Gokul, I am assigning the bug to you. Wesj is the mentor on this bug. Feel free to request any information.

Comment 5

[Gokul Caushik]

Attachment: Proposed patch for Bug 972723

Comment 6

[Gokul Caushik]

I added a lot of debug messages to see where the code paths are.
The issue seems to be due to a race condition in the "pageshow" event handler in the browser.js file.

In the normal case when the url chosen for a tab has a "readable" article, the callback from the Reader.parseDocumentFromTab method (http://lxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3999) sets the readerEnabled attribute to true which ends up adding pageActions to the urlbar.

However what caused the bug was when the android back key was being pressed, soon after the url with the readable article begins to load, the "pageshow" event is triggered for the (config:home) page, BEFORE the callback from the previous entry to the page with the article happens.

As a result the updatePageAction to exit and  "Remove" the reader mode happens FOLLOWED by the callback for the previous call to Reader.parseDocumentFromTab which ends up turning readerEnabled back ON and updatePageAction to enter reader mode.

The proposed fix is a simple one line change to compare the tabURL of the article to the tabURL of the current BrowserApp (when the callback actually happens) rather than the URL that existed when the call to Reader.parseDocumentFromTab is made.

I tested this manually and it seemed to fix the bug.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8424583 [details] [diff] [review]
Proposed patch for Bug 972723

Review of attachment 8424583 [details] [diff] [review]:
-----------------------------------------------------------------

Yay! Nice and thanks for the detective work! These flows can get really confusing, so I'm impressed you tracked this race down :)

::: mobile/android/chrome/content/browser.js
@@ +3998,5 @@
>          // Once document is fully loaded, parse it
>          Reader.parseDocumentFromTab(this.id, function (article) {
>            // Do nothing if there's no article or the page in this tab has
>            // changed
> +          let uri = this.browser.currentURI;

You might add a small comment to make sure no one else accidentally changes this. Something like

// The loaded page may have changed while we were parsing the document. Make sure we've got the current one.

Comment 8

[Gokul Caushik]

No problem, how should I do this, should I create another patch for the comment?

Comment 9

[Aaron Train [:aaronmt]]

Yes.

Comment 10

[Gokul Caushik]

Attachment: Updated patch with bug fix and comment.

There were some issues creating a patch after hg merge so I just created new copy of repo and did all the changes (both the bug fix and comment) in one patch.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 8426024 [details] [diff] [review]
Updated patch with bug fix and comment.

Review of attachment 8426024 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Thanks!

If you can want, you can make up this patch for checkin:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

upload it (mark it r+) and then add 'checkin-needed' to the keywords for the bug. Otherwise, I'll just update it and land sometime soon.

Comment 12

[Gokul Caushik]

Attachment: 972723.patch

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0eb37d44478