Fix error fetching favicon during background updates.

[Osmose]

Small fix for a potential error that I'm hitting on the fake product page from the favicon changes.

[biancadanforth]

R(+WC?) Thanks Osmose for taking a moment to try and address this error. I'm not sure what error you're seeing, but the error I'm seeing doesn't go away with this patch. I suggest a minor change that removes the error from my end.

[biancadanforth]

I am not sure this is exactly the error you were seeing, but this is the error I have been seeing, and I am still seeing in this patch:

[Exception... "Favicon at "http://www.mkelly.me/favicon.ico" failed to load: Not Found."  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///modules/FaviconLoader.jsm :: onStopRequest :: line 154"  data: no]

I think this is because sender.tab.favicon can also be undefined in the case that the tab is still loading (though the docs say it should return '') or the site (like the fake product page) doesn't have a favicon.

If I change it to:

Suggested change

	vendorFaviconUrl: sender.tab ? sender.tab.favIconUrl : '',
	vendorFaviconUrl: (sender.tab && sender.tab.favIconUrl) ? sender.tab.favIconUrl : '',

I no longer see the error.

[Osmose]

I am not sure this is exactly the error you were seeing,

I was seeing both an error that sender.tab was null, and the error you're seeing. My original PR fixed the former, but not the latter. I thought the latter was just an error that all pages without favicons gave and was unavoidable. Interesting to find that it wasn't.

[Osmose]

I tried this change out locally and it doesn't remove the error.

I think this is because sender.tab.favicon can also be undefined in the case that the tab is still loading (though the docs say it should return '') or the site (like the fake product page) doesn't have a favicon.

The error you're seeing isn't an error about the value being undefined. The error message says that it failed to load favicon.ico from the webpage; it's not an error in how we're accessing the attribute, it's an error in loading the icon file. As long as we're accessing the favicon for a page that doesn't have one, that error will show up in the browser console.

For what it's worth, I'm not seeing any behavior differences because of it. Can you verify that you're seeing breakage from specifically that error?

[biancadanforth]

Yeah it doesn't look like it breaks anything; it continues executing the handleExtractedProductData code downstream of the error.

[biancadanforth]

@Osmose I think this error is actually breaking price updates currently, so we may want to tag this commit and push a new version of the extension after merging? I'm working on #166 and I didn't see any price alerts for the fake product page until I made this change (my recommended change in particular).

[Osmose]

I'm working on #166 and I didn't see any price alerts for the fake product page until I made this change (my recommended change in particular).

This PR fixes price alerts for me as well, although the extra change you suggested doesn't appear to have an effect (see my pending comment above).

[Osmose]

Thanks for the review! I landed this so we could get out the extraction fix, but we may still followup with another fix if the other error turns out to be a problem.