Closed Bug 1260276 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Reader Mode, defect)

Product:
Toolkit
Component:
Reader Mode
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox48 --- affected
firefox50 --- verified

People

(Reporter: arni2033, Assigned: timdream)

References

Details

Attachments

(1 file)

Bug 1260276 - Replace the dead reader page in history when we are being redirected,
58 bytes, text/x-review-board-request
Gijs
: review+
Details 
>>>   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.
(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 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?
(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/
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
https://hg.mozilla.org/mozilla-central/rev/807806313c03
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Depends on: 1344431
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: