favicon.path is undefined when clicking reader-mode-button

VERIFIED FIXED in Firefox 59

Status

()

VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: magicp.jp, Assigned: Gijs)

Tracking

(Depends on: 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox58 wontfix, firefox59 verified, firefox60 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Steps to reproduce:
1. Launch Nightly
2. Go to "https://developer.mozilla.org/en-US/Firefox"
3. Click reader-mode-button

Actual Results:
Logged an error in Browser Console as below.
favicon.path is undefined  ReaderParent.jsm:47
(Assignee)

Updated

a year ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 1

a year ago
I know I should have picked this up sooner but I've just been distracted by other stuff. Looking at this just now for unrelated reasons, I can't help but wonder... is this a regression? Is it possible for you to check this? It looks like this is using a places API that should be returning an object with a .path property but is no longer doing so, so I wonder if this is a regression caused by a places API change or something. Having a regression window would make looking for that stuff a lot easier.
Flags: needinfo?(magicp.jp)
Probably bug 1326520 that changed nsIURI.path to nsIURI.pathQueryRef
(Assignee)

Comment 3

a year ago
(In reply to Marco Bonardo [::mak] from comment #2)
> Probably bug 1326520 that changed nsIURI.path to nsIURI.pathQueryRef

Oh irony, that that updated Readability.js (which it shouldn't have done) but not ReaderParent.jsm (which it should have done).
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1326520
Status: NEW → ASSIGNED
status-firefox58: --- → wontfix
status-firefox60: --- → affected
Flags: needinfo?(magicp.jp)
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1433958
No longer blocks: 1426501
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
Not sure what this has to do with the setters bug - we're only touching getters here, I think? :-)
Flags: needinfo?(valentin.gosu)
My bad. I saw .replace and I thought it was actually changing it :)
No longer blocks: 1433958
Flags: needinfo?(valentin.gosu)

Comment 7

a year ago
mozreview-review
Comment on attachment 8946671 [details]
Bug 1420714 - fix favicon fetching in reader mode,

https://reviewboard.mozilla.org/r/216648/#review222366

LGTM, but I have a question.

::: toolkit/components/reader/test/browser_readerMode.js:55
(Diff revision 1)
>    let readerUrl = gBrowser.selectedBrowser.currentURI.spec;
>    ok(readerUrl.startsWith("about:reader"), "about:reader loaded after clicking reader mode button");
>    is_element_visible(readerButton, "Reader mode button is present on about:reader");
> +  let favicon = document.getAnonymousElementByAttribute(tab, "anonid", "tab-icon-image");
> +  is_element_visible(favicon, "Favicon should be visible");
> +  is(favicon.src, TEST_PATH + "favicon.ico", "Correct favicon should be loaded");

I don't know the reader code very well, should this wait on some event to avoid intermittents or use waitForCondition? what prevents it from checking the favicon before it has been fetched and stored?
Attachment #8946671 - Flags: review?(mak77) → review+
(Assignee)

Comment 8

a year ago
(In reply to Marco Bonardo [::mak] from comment #7)
> Comment on attachment 8946671 [details]
> Bug 1420714 - fix favicon fetching in reader mode,
> 
> https://reviewboard.mozilla.org/r/216648/#review222366
> 
> LGTM, but I have a question.
> 
> ::: toolkit/components/reader/test/browser_readerMode.js:55
> (Diff revision 1)
> >    let readerUrl = gBrowser.selectedBrowser.currentURI.spec;
> >    ok(readerUrl.startsWith("about:reader"), "about:reader loaded after clicking reader mode button");
> >    is_element_visible(readerButton, "Reader mode button is present on about:reader");
> > +  let favicon = document.getAnonymousElementByAttribute(tab, "anonid", "tab-icon-image");
> > +  is_element_visible(favicon, "Favicon should be visible");
> > +  is(favicon.src, TEST_PATH + "favicon.ico", "Correct favicon should be loaded");
> 
> I don't know the reader code very well, should this wait on some event to
> avoid intermittents or use waitForCondition? what prevents it from checking
> the favicon before it has been fetched and stored?

conversely, I don't know places and how the favicon gets fetched and stored. :-)

We load the regular page and wait for the load, then wait for the reader mode icon to be visible, then load reader view. Is that likely to be problematic? It worked in my local testing, but that's not a very good guarantee that it'll always work...

Is there a good way to wait for the favicon to be present?
Flags: needinfo?(mak77)
> (In reply to Marco Bonardo [::mak] from comment #7)
> We load the regular page and wait for the load, then wait for the reader
> mode icon to be visible, then load reader view. Is that likely to be
> problematic? It worked in my local testing, but that's not a very good
> guarantee that it'll always work...

I think it boils down to whether all of that work is longer than fetching the icon and storing it, that is all asynchronous and depending on I/O it could actually happen at any time... I can't predict whether in practice it will be an issue on tinderboxes or not.
Additionally, it's somehow possible that the reader view itself is not waiting long enough and in some cases will just push out the default icon instead of the right one, though it's likely the user is a bit slower than some test code.

> Is there a good way to wait for the favicon to be present?

One way could be to add an history observer and wait for onPageChanged with ATTRIBUTE_FAVICON (in the future we'll have a better single-target notification) for the page url.
Another way could be to just poll with waitForCondition in the test.
A third possibility could be, in the test, to previously store the icon in the favicons service for that page rather than through the <link>, so it's readily available. For example https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/browser/components/places/tests/browser/browser_toolbar_overflow.js#83
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
As discussed on IRC, worked around the 'waiting' issue by explicitly loading the favicon into places instead of doing it via a pageload. As a bonus, now I don't need external files so the patch is much simpler! :-)

Comment 12

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/84474df16e80
fix favicon fetching in reader mode, r=mak

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84474df16e80
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is this worth uplifting to 59?
status-firefox59: affected → fix-optional
(Assignee)

Comment 15

a year ago
Comment on attachment 8946671 [details]
Bug 1420714 - fix favicon fetching in reader mode,

(In reply to Julien Cristau [:jcristau] from comment #14)
> Is this worth uplifting to 59?

Yes, good point.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1326520
[User impact if declined]: no favicons for reader mode, errors in the browser console
[Is this code covered by automated tests?]: it is now! I added some in the patch.
[Has the fix been verified in Nightly?]: I verified manually while writing the patch, and there's automated tests, but it hasn't been independently verified, if that matters...
[Needs manual test from QE? If yes, steps to reproduce]: tbh, given manual verification + automated tests, not sure it's helpful, but if necessary steps are in comment #0.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: 1-line code change, has automated test, has baked for a bit now
[String changes made/needed]: nope
Attachment #8946671 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Flags: in-testsuite+
Comment on attachment 8946671 [details]
Bug 1420714 - fix favicon fetching in reader mode,

Simple fix with new tests added. Let's take this for 59b8.
Attachment #8946671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5924d4660bad
status-firefox59: fix-optional → fixed
I have managed to reproduce the issue described in comment 0 using Firefox 59.0a1 (BuildId:20171126220311).

This issue is verified fixed using Firefox 60.0a1 (BuildId:20180208103049) and Firefox 59.0b8 (BuildId:20180208125751 -Tinderbox build) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 14.04 64bit.
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
status-firefox60: fixed → verified
Flags: qe-verify+
Depends on: 1453467
You need to log in before you can comment on or make changes to this bug.