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

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[magicp]

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

Comment 1

[:Gijs (he/him)]

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.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Probably bug 1326520 that changed nsIURI.path to nsIURI.pathQueryRef

Comment 3

[:Gijs (he/him)]

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

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1420714 - fix favicon fetching in reader mode,

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

Comment 5

[:Gijs (he/him)]

Not sure what this has to do with the setters bug - we're only touching getters here, I think? :-)

Comment 6

[Valentin Gosu [:valentin] (he/him)]

My bad. I saw .replace and I thought it was actually changing it :)

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 8

[:Gijs (he/him)]

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

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 10

[:Gijs (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216648/diff/1-2/

Comment 11

[:Gijs (he/him)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84474df16e80
fix favicon fetching in reader mode, r=mak

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/84474df16e80

Comment 14

[Julien Cristau [:jcristau]]

Is this worth uplifting to 59?

Comment 15

[:Gijs (he/him)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5924d4660bad

Comment 18

[Emil Ghitta, QA [:emilghitta]]

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.