User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0 Build ID: 20150202030232 Steps to reproduce: Open a webpage this affects - example is https://bugzilla.mozilla.org/enter_bug.cgi#h=dupes|Firefox. Click on the reader mode button in the address bar. Actual results: There is no actual reader view. The about:reader page just redirects back to the non-reader mode page. Expected results: The button should not display when there is no reader mode available for the page. Either the about:reader page should show an actual reader mode, or the reader mode button should not appear at all.

I can reproduce this. I suspect the problem here probably has to do with creating the url parameter in the about:reader URL. What's likely happening is that there *is* a reader mode article, but that when we load about:reader, we run into some issue with the URL. I can investigate.

I can reproduce this with any URL containing a fragment e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1128757#c0

Oh, maybe this is the same issue as bug 1135084. Sorry I haven't had time to dig into this yet :(

What's happening here is that we drop the ref part of the article url: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?force=1#223 So when we do a comparison of the current url to the saved article's url to make sure we're loading the correct thing, we find they are not equal, and we think we didn't find an article. I have a patch that will fix this issue as we see it now, but this actually exposes a bigger problem for articles like the one I saw in bug 1135084: http://m.thestar.com/#/article/news/gta/2015/02/19/horror-hope-and-heartbreak-for-our-lost-child-elijah-keenan.html In this case, the ref actually influences the content of the page (/me shakes fist at web developers), and what's extra unfortunate is that we're using this stripped URL for caching content: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?force=1#156 This makes me think that the real fix here is to get rid of any .specIgnoringRef calls, and just pass around complete URLs as we see them. The unfortunate thing here would be that https://bugzilla.mozilla.org/show_bug.cgi?id=1128757#c0 and https://bugzilla.mozilla.org/show_bug.cgi?id=1128757#c1 would be cached as different articles, but we only actually use this cache when you save URLs to it, and if you save both of those URLs, I'm fine with us just caching that content twice. rnewman/bnicholson, what do you two think we should do here?

/r/4391 - Bug 1128757 - Trim original url before trying to get article for url Pull down this commit: hg pull review -r cde1276a9f38fe14e33a35e6eefa556cb8d5f254

(In reply to :Margaret Leibovic from comment #6) > Created attachment 8570157 [details] > MozReview Request: bz://1128757/margaret > > /r/4391 - Bug 1128757 - Trim original url before trying to get article for > url > > Pull down this commit: > > hg pull review -r cde1276a9f38fe14e33a35e6eefa556cb8d5f254 This is my WIP that fixes toggling reader view from the toolbar button, but we should think about whether or not this is the correct approach.

(In reply to :Margaret Leibovic from comment #5) > This makes me think that the real fix here is to get rid of any > .specIgnoringRef calls, and just pass around complete URLs as we see them. ... > rnewman/bnicholson, what do you two think we should do here? We have the DOM, so in theory we can determine whether the fragment identifier refers to an identifier that exists in the page. I'd err on the side of keeping the entire URL unchanged, but if we ever discard it, that knowledge might help.

I agree, I'd prefer potentially duplicate saved articles over broken ones.

Comment on attachment 8570157 [details] MozReview Request: bz://1128757/margaret /r/4391 - Bug 1128757 - Do not trim fragments from URLs loaded in reader view. r=bnicholson Pull down this commit: hg pull review -r 9b50a3cb5b00aefe875935e5cad136879b6f8271

This change depends on bug 1134443.

Comment on attachment 8570157 [details] MozReview Request: bz://1128757/margaret https://reviewboard.mozilla.org/r/4389/#review3749 Ship It!

One thing to consider here is that we may now end up with an inconsistency between article URLs in the reading list and URLs for articles stored in the cache... However, I *think* it will be okay because we were stripping the fragment from the article URL before sending the article over to be stored in Java, so in theory what is stored in the cache will match the article that is stored in the content provider. I'll think about this a bit more. In the worst case, we wouldn't find an article in the cache for an article, and we may end up with orphan articles in the cache. But hopefully bug 1126244 can help deal with that.