Closed Bug 1472438 Opened 3 years ago Closed 3 years ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 63.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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?
Blocks: 1469374
Flags: needinfo?(mak77)
Flags: needinfo?(dtownsend)
Attached patch 1472438-disable-tests.patch (obsolete) — Splinter Review
(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.
Flags: needinfo?(dtownsend)
Dave, thanks for the comment and please accept my sincere apologies for blaming it on M-C when I didn't even look properly :-(
Flags: needinfo?(mak77)
No worries, I should have checked and given you a heads up first anyway.
Attached patch 1472438-isFailedFavicon.patch (obsolete) — Splinter Review
Test passes like this and I can see the two icons (star and exclamation mark) again.
Assignee: nobody → jorgk
Attachment #8989008 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8989015 - Flags: review?(acelists)
Further simplification, sorry about the noise.
Attachment #8989015 - Attachment is obsolete: true
Attachment #8989015 - Flags: review?(acelists)
Attachment #8989016 - Flags: review?(acelists)
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?
Attachment #8989016 - Flags: feedback?(dtownsend)
Aceman asked me on IRC: "how did they [M-C] explain the removal [of the interfaces]?"
Attachment #8989016 - Flags: feedback?(dtownsend) → feedback+
(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 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.
Attachment #8989016 - Flags: review?(acelists) → review+
(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.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/864d0a946940
Remove use of nsIFaviconService.isFailedFavicon() after its removal in bug 1469374. r=aceman f=mossop
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
You need to log in before you can comment on or make changes to this bug.