Reader mode messes with tab history when I navigate to article in reader mode (on facebook articles)

VERIFIED FIXED in Firefox 50

Status

()

Toolkit
Reader Mode
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: arni2033, Assigned: timdream)

Tracking

Trunk
mozilla50
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox48 affected, firefox50 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Comment 2

a year ago
>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
Somehow comment 0 and comment 1 no longer work (huh). But I still see the mess.

STR_3:
1. Open new tab https://www.facebook.com/permalink.php?story_fbid=209620482737575&id=100010688735430
2. Click reader button in urlbar
3. Reload the page
4. Go Back in tab history (Alt+Left)

AR:  Blank page
ER:  Original page
Whiteboard: read comment 2 first
I can reproduce your comment 2 STR. On step 3, some how we would create a new history entry. This should not happen.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Whiteboard: read comment 2 first
The root cause of this bug is because the code here incorrectly parsed and followed the <meta> tag being wrapped inside a <noscript>

http://searchfox.org/mozilla-central/rev/c44d0b1673fef5e0e2e19fa82d6780a74c186151/toolkit/components/reader/ReaderMode.jsm#242-272

When we first load the page, the document was parsed directly from the loaded source and not xhr'd here.

Proposed fix: When follow a meta refresh, we should replace, not append a new history item.

This also "fix" another issue where the page being redirected is forever trapped in the loading (blank) state. User should not be able to reach that with the back button.
Created attachment 8770443 [details]
Bug 1260276 - Replace the dead reader page in history when we are being redirected,

Review commit: https://reviewboard.mozilla.org/r/63902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63902/
Attachment #8770443 - Flags: review?(gijskruitbosch+bugs)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> The root cause of this bug is because the code here incorrectly parsed and
> followed the <meta> tag being wrapped inside a <noscript>
> 
> http://searchfox.org/mozilla-central/rev/
> c44d0b1673fef5e0e2e19fa82d6780a74c186151/toolkit/components/reader/
> ReaderMode.jsm#242-272
> 
> When we first load the page, the document was parsed directly from the
> loaded source and not xhr'd here.

Note that I didn't not proposed a solution to stop the redirection of <noscript><meta/></noscript> because I don't think having a behavior here with based <noscript> doesn't really make sense (the page can be parsed anyway).

Comment 7

a year ago
Comment on attachment 8770443 [details]
Bug 1260276 - Replace the dead reader page in history when we are being redirected,

https://reviewboard.mozilla.org/r/63902/#review60960

r=me with the error case addressed.

::: toolkit/components/reader/AboutReader.jsm:640
(Diff revision 1)
>        // If we were promised an article, show an error message if there's a failure.
>        this._showError();
>      } else {
>        // Otherwise, just load the original URL. We can encounter this case when
>        // loading an about:reader URL directly (e.g. opening a reading list item).
> -      this._win.location.href = url;
> +      this._win.location.replace(url);

This particular change doesn't seem right. If we're replacing the readerized version with the non-readerized version of the same document, doesn't that mean we can go back in history? We shouldn't replace the reader mode item with the identical URL that was already in history, or it'll just lead to two identical history entries...
Attachment #8770443 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #7)
> ::: toolkit/components/reader/AboutReader.jsm:640
> (Diff revision 1)
> >        // If we were promised an article, show an error message if there's a failure.
> >        this._showError();
> >      } else {
> >        // Otherwise, just load the original URL. We can encounter this case when
> >        // loading an about:reader URL directly (e.g. opening a reading list item).
> > -      this._win.location.href = url;
> > +      this._win.location.replace(url);
> 
> This particular change doesn't seem right. If we're replacing the readerized
> version with the non-readerized version of the same document, doesn't that
> mean we can go back in history? We shouldn't replace the reader mode item
> with the identical URL that was already in history, or it'll just lead to
> two identical history entries...

You are right, it's just ... convenient. We would still need to prevent the error'd reader page to be accessible in the history; if we really care about not creating duplicating entries, let me change this line to 

ReaderMode.leaveReaderMode(this._mm.docShell, this._win);

?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #8)
> You are right, it's just ... convenient. We would still need to prevent the
> error'd reader page to be accessible in the history; if we really care about
> not creating duplicating entries, let me change this line to 
> 
> ReaderMode.leaveReaderMode(this._mm.docShell, this._win);
> 
> ?

Actually, no. This prevents the duplicate entry, but it would keep the error'd, bfcache'd reader page in the history. Maybe we should just `this._showError();` here?

Comment 10

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #8)
> > You are right, it's just ... convenient. We would still need to prevent the
> > error'd reader page to be accessible in the history; if we really care about
> > not creating duplicating entries, let me change this line to 
> > 
> > ReaderMode.leaveReaderMode(this._mm.docShell, this._win);
> > 
> > ?
> 
> Actually, no. This prevents the duplicate entry, but it would keep the
> error'd, bfcache'd reader page in the history. Maybe we should just
> `this._showError();` here?

This seems like the most sensible option, yes.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8770443 [details]
Bug 1260276 - Replace the dead reader page in history when we are being redirected,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63902/diff/1-2/
Keywords: checkin-needed

Comment 12

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/807806313c03
Replace the dead reader page in history when we are being redirected, r=gijs
Keywords: checkin-needed

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/807806313c03
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: qe-verify+

Comment 14

a year ago
I was able to reproduce the issue using STR provided in comment 2, with an old Nightly 50.0a1 (2016-07-08) build, on Windows 10 Pro x64.

The fix was verified on Firefox Beta 50.0b4 across platforms: Windows 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Sierra 10.12.1

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified

Updated

9 months ago
Depends on: 1344431
You need to log in before you can comment on or make changes to this bug.