>>> 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

I can reproduce your comment 2 STR. On step 3, some how we would create a new history entry. This should not happen.

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.

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

(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...

(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); ?

(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.

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

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