1472438 - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-tabs/test-content-tab.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

WARNING - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_open
WARNING - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_default_favicon

First seen Sat, Jun 30, 2018, 11:13:51
M-C last good: 28ad9a9e95d518e1163e550ae19c972aabb44df5
M-C first bad: 19766d4c54e3c0ef09caf3c9a7fd3f162e4d5ac6
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=28ad9a9e95d518e1163e550ae19c972aabb44df5&tochange=19766d4c54e3c0ef09caf3c9a7fd3f162e4d5ac6

Could be:
a2441e692827	Dave Townsend — Bug 1469374: Remove code that tracks failed favicon loads. r=mak

INFO -    EXCEPTION: The tab [[object Object]] should have a favicon with URL http://localhost:43336/whatsnew.png but instead has null
INFO -      at: test-folder-display-helpers.js line 107
INFO -         do_throw test-folder-display-helpers.js:107 13
INFO -         mark_failure logHelper.js:650 3
INFO -         assert_content_tab_has_favicon test-content-tab-helpers.js:288 5
INFO -         test_content_tab_open test-content-tab.js:41 3

INFO -  SUMMARY-UNEXPECTED-FAIL | test-content-tab.js | test-content-tab.js::test_content_tab_default_favicon
INFO -    EXCEPTION: The tab [[object Object]] should have a favicon with URL http://localhost:43336/favicon.ico but instead has null
INFO -      at: test-folder-display-helpers.js line 107
INFO -         do_throw test-folder-display-helpers.js:107 13
INFO -         mark_failure logHelper.js:650 3
INFO -         assert_content_tab_has_favicon test-content-tab-helpers.js:288 5
INFO -         test_content_tab_default_favicon test-content-tab.js:147 3

Comment 1

[Jorg K (CEST = GMT+2)]

Ran
  mozmake SOLO_TEST=content-tabs/test-content-tab.js mozmill-one
locally and the two tabs that the test opens have two plain "document" icons.

With M-C rev a2441e692827 (bug 1469374) backed out the test passes and I see different icons, a star and an exclamation mark. Note that predecessor bug 1453751 which landed prior to the regression range didn't cause any damage.

Marco and Dave, somehow your changes broke icon loading in TB. Looks like there are also other regressions, bug 1472413.

I can see that https://hg.mozilla.org/mozilla-central/rev/a2441e692827 removed a bunch of interfaces from the IDL, like *FailedFavicon(), but we're not using those in our test:
https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/content-tabs/test-content-tab.js

To me it looks like the Mozilla platform simply broke here and there's nothing we can do to adapt to that change. Or am I missing something?

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1472438-disable-tests.patch

Comment 3

[Dave Townsend [:mossop]]

(In reply to Jorg K (GMT+2) from comment #1)
> Ran
>   mozmake SOLO_TEST=content-tabs/test-content-tab.js mozmill-one
> locally and the two tabs that the test opens have two plain "document" icons.
> 
> With M-C rev a2441e692827 (bug 1469374) backed out the test passes and I see
> different icons, a star and an exclamation mark. Note that predecessor bug
> 1453751 which landed prior to the regression range didn't cause any damage.
> 
> Marco and Dave, somehow your changes broke icon loading in TB. Looks like
> there are also other regressions, bug 1472413.
> 
> I can see that https://hg.mozilla.org/mozilla-central/rev/a2441e692827
> removed a bunch of interfaces from the IDL, like *FailedFavicon(), but we're
> not using those in our test:
> https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/content-tabs/
> test-content-tab.js
> 
> To me it looks like the Mozilla platform simply broke here and there's
> nothing we can do to adapt to that change. Or am I missing something?

Thunderbird is using the methods that we removed from the interface, for example https://searchfox.org/comm-central/source/mail/base/content/specialTabs.js#184 is probably the source of the problem here. If you remove calls to those methods I would expect it to fix things. The only impact is that you might load favicons more often, which is what we're doing in Firefox, but I would expect the image cache to cut that down anyway.

Comment 4

[Jorg K (CEST = GMT+2)]

Dave, thanks for the comment and please accept my sincere apologies for blaming it on M-C when I didn't even look properly :-(

Comment 5

[Dave Townsend [:mossop]]

No worries, I should have checked and given you a heads up first anyway.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1472438-isFailedFavicon.patch

Test passes like this and I can see the two icons (star and exclamation mark) again.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1472438-isFailedFavicon.patch

Further simplification, sorry about the noise.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8989016 [details] [diff] [review]
1472438-isFailedFavicon.patch

Hi Dave, you're still around, so perhaps you can provide feedback here. Straight removal is the way to go, right?

Comment 9

[Jorg K (CEST = GMT+2)]

Aceman asked me on IRC: "how did they [M-C] explain the removal [of the interfaces]?"

Comment 10

[Dave Townsend [:mossop]]

(In reply to Jorg K (GMT+2) from comment #9)
> Aceman asked me on IRC: "how did they [M-C] explain the removal [of the
> interfaces]?"

We decided that it was an unnecessary complication. What it does is stop multiple requests in the event that a site has no favicon or a broken link to a favicon. This should be rare. Since we load favicons in the content process in Firefox now a failed icon cache would need to be share across processes which looked too complex to be worth it.

Comment 11

[:aceman]

Comment on attachment 8989016 [details] [diff] [review]
1472438-isFailedFavicon.patch

Review of attachment 8989016 [details] [diff] [review]:
-----------------------------------------------------------------

So without the cache if we hit a site with broken icon, we may try to fetch it more times until it is found broken. But if you said that the image cache should then cut these requests.

Comment 12

[Dave Townsend [:mossop]]

(In reply to :aceman from comment #11)
> Comment on attachment 8989016 [details] [diff] [review]
> 1472438-isFailedFavicon.patch
> 
> Review of attachment 8989016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So without the cache if we hit a site with broken icon, we may try to fetch
> it more times until it is found broken. But if you said that the image cache
> should then cut these requests.

XUL:image elements use an image cache to avoid loading the same image multiple times. I haven't dug into it too much but it should reduce the requests regardless of whether the icon failed to load or not.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/864d0a946940
Remove use of nsIFaviconService.isFailedFavicon() after its removal in bug 1469374. r=aceman f=mossop