Closed Bug 1804166 Opened 2 years ago Closed 15 days ago

favicon not reseting when changing tab to about:newtab with newtab to Blank Page

Categories

(Firefox :: Tabbed Browser, defect, P5)

Product:
Firefox
Component:
Tabbed Browser
Version:
Firefox 106
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
firefox130 --- wontfix
firefox131 --- verified

People

(Reporter: 080ariel, Assigned: timw-bugzilla, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(2 files)

Bug 1804166 - favicon not reseting when changing tab to about:newtab with newtab to Blank Page.r?tgiles!
48 bytes, text/x-phabricator-request
Details | Review
video showing the issue
1.48 MB, video/webm
Details

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:106.0) Gecko/20100101 Firefox/106.0

Steps to reproduce:

  1. Go to settings and change New Tab to Blank Page(I don't know if this is necessary)
  2. Open a new tab and go to www.google.com
  3. Change that tab location to about:newtab

Actual results:

The favicon(small icon in the tab) is the www.google.com one.

Expected results:

The favicon(small icon in the tab) is the about:newtab one (Firefox logo).

The Bugbug bot thinks this bug should belong to the 'Firefox::New Tab Page' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → New Tab Page

Hello, thank you for the bug report!
I managed to reproduce this issue using the STR from the reporter on:

  • Firefox 107.0;
  • Firefox 108.0;
  • Nightly 109.0a1;

Tested and reproduced on:

  • macOS 12;
  • Windows 10;
  • Ubuntu 22;

Setting as NEW so that the developers can have a look.

Status: UNCONFIRMED → NEW
Ever confirmed: true

The severity field is not set for this bug.
:daleharvey, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dharvey)

Clearing needinfos as triage owner has switched (apologies for any late replies, was not overly familiar with this area of code)

Flags: needinfo?(dharvey)

It does appear that having the newtab page be set to about:blank is necessary for this to occur.

I don't think this really has to do with about:newtab, and more to do with the tabbrowser code that's responsible for showing favicons after navigation. The mIconURL is correctly updating to null after loading the blank about:newtab, but it doesn't seem that tabbrowser is reflecting this.

Severity: -- → S4
Component: New Tab Page → Tabbed Browser
Priority: -- → P3
Priority: P3 → --

Clearing priority / severity for tabbrowser triage.

Severity: S4 → --
Severity: -- → S4
Priority: -- → P5
Mentor: tgiles
Keywords: good-first-bug
Whiteboard: [lang=js]
Assignee: nobody → timw-bugzilla

Hello,

Able to replicate bug in Firefox Version 127.0.2 on Ubuntu 22 . Following up on Mike Conley's comment above, I am currently looking into occurrences of mIconURL in browser/components/tabbrowser/content/tabbrowser.js . Not sure how to verify statement: "The mIconURL is correctly updating to null after loading the blank about:newtab, but it doesn't seem that tabbrowser is reflecting this."

Flags: needinfo?(tgiles)

(In reply to Tim Williams from comment #7)

Hello,

Able to replicate bug in Firefox Version 127.0.2 on Ubuntu 22 . Following up on Mike Conley's comment above, I am currently looking into occurrences of mIconURL in browser/components/tabbrowser/content/tabbrowser.js . Not sure how to verify statement: "The mIconURL is correctly updating to null after loading the blank about:newtab, but it doesn't seem that tabbrowser is reflecting this."

Hi Tim,

I'm a new contributor who'd like to work on this bug. I am looking for some additional context/information to move forward (tried to edit my original comment, but couldn't figure out how).

Regards,
Tim

Hi Tim, was chasing down my notes for this bug, apologies.

Not sure how to verify statement: "The mIconURL is correctly updating to null after loading the blank about:newtab, but it doesn't seem that tabbrowser is reflecting this."

You can verify that mIconURL is changing by adding a debugger statement or breakpoint to this line in tabbrowser.js where we set mIconURL to null. You'll need to use the browser toolbox to verify this as opposed to the regular web content devtools. If you haven't used the browser toolbox before, see this document explaining how to enable the browser toolbox.

How I would recommend verifying is by doing the following:

  1. Go to settings and change New Tab to Blank Page
  2. Open a new tab and go to google.com
  3. Open the browser toolbox and put a breakpoint on that line I mentioned earlier in this comment
  4. Change the google.com tab to about:newtab

This should trigger the breakpoint so that you can see what's going on. Let me know if you get stuck on trying to figure this out, I do have a potential solution that appears to fix the issue but I haven't thoroughly tested it.

Flags: needinfo?(tgiles) → needinfo?(timw-bugzilla)

No worries, thank you for your guidance. Following your steps I was able to verify that mIconURL is indeed assigned a value of null after browsing from google.com to about:newtab .

Yes, I am feeling stuck and would like to see your potential solution.

Flags: needinfo?(timw-bugzilla) → needinfo?(tgiles)

This is the potential solution I came up with:

diff --git a/browser/components/tabbrowser/content/tabbrowser.js b/browser/components/tabbrowser/content/tabbrowser.js
index b26166181e8e1..82e74844c2708 100644
--- a/browser/components/tabbrowser/content/tabbrowser.js
+++ b/browser/components/tabbrowser/content/tabbrowser.js
@@ -15,6 +15,7 @@
     "about:welcome": "chrome://branding/content/icon32.png",
     "about:privatebrowsing":
       "chrome://browser/skin/privatebrowsing/favicon.svg",
+    "about:blank": "chrome://branding/content/icon32.png",
   };
 
   const {
@@ -7142,6 +7143,9 @@
             // Removing the tab's image here causes flickering, wait until the
             // load is complete.
             this.mBrowser.mIconURL = null;
+
+            // Bug 1804166: Hack to get about:blank to update its icon accordingly
+            gBrowser.setDefaultIcon(this.mTab, this.mBrowser._documentURI);
           }
 
           if (!isReload && aWebProgress.isLoadingDocument) {

It definitely feels like a hack, but might work for solving this issue.

Flags: needinfo?(tgiles)

Thank you for that, your solution worked for me on my end. Are there any tests I should run locally ?

( On a side note, it was helpful for me to read up on gBrowser here . It helped me to wrap my head around mBrowser and mTab as properties inside the TabProgressListener instance that reference the actual browser and tab objects. )

Flags: needinfo?(tgiles)

(In reply to Tim Giles [:tgiles] from comment #9)

How I would recommend verifying is by doing the following:

  1. Go to settings and change New Tab to Blank Page
  2. Open a new tab and go to google.com
  3. Open the browser toolbox and put a breakpoint on that line I mentioned earlier in this comment
  4. Change the google.com tab to about:newtab

I'm a bit confused here; my understanding is that even with the Blank Page setting, new tabs will still use about:newtab which would then point to a blank page. So if none of the above steps involve about:blank, why does your potential solution involve adding that to FAVICON_DEFAULTS?

(In reply to Dão Gottwald [:dao] from comment #14)

(In reply to Tim Giles [:tgiles] from comment #9)

How I would recommend verifying is by doing the following:

  1. Go to settings and change New Tab to Blank Page
  2. Open a new tab and go to google.com
  3. Open the browser toolbox and put a breakpoint on that line I mentioned earlier in this comment
  4. Change the google.com tab to about:newtab

I'm a bit confused here; my understanding is that even with the Blank Page setting, new tabs will still use about:newtab which would then point to a blank page. So if none of the above steps involve about:blank, why does your potential solution involve adding that to FAVICON_DEFAULTS?

Ah, I believe I got about:blank mixed up with the about:newtab blank page behavior. You're absolutely correct that we don't need to add an entry to FAVICON_DEFAULTS in this case. Thanks for double checking that, I appreciate it!

Ok, thanks for the catch! Submitted a new revision with the about:blank property and its associated value removed from FAVICON_DEFAULTS.

(In reply to Tim Williams from comment #13)

Thank you for that, your solution worked for me on my end. Are there any tests I should run locally ?

I would run the tests under browser/base/content/test/favicons. I'm not able to easily find tests that directly relate to the problem we're trying to solve, but making sure we don't break favicons elsewhere should be a good first step in finding a test for this situation. Maybe also run browser_tabicon_after_bg_tab_crash.js as well (since that is checking mIconUrl stuff). We'll probably need to write a new test for this exact situation. I'm not aware of tests that change the about:newtab behavior, so we'll need to look for any tests that do something similar.

Flags: needinfo?(tgiles)
Attachment #9412389 - Attachment description: Bug 1804166 - favicon not reseting when changing tab to about:newtab with newtab to Blank Page.r?tgiles → Bug 1804166 - favicon not reseting when changing tab to about:newtab with newtab to Blank Page.r?tgiles!

Ok, thank you for that. All tests under browser/base/content/test/favicons ran with no errors, as well as browser_tabicon_after_bg_tab_crash.js

I have submitted a revision to Phabricator with :dao 's requested changes.

Flags: needinfo?(tgiles)
Pushed by tgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da2dd0daaa35
favicon not reseting when changing tab to about:newtab with newtab to Blank Page.r=tgiles,tabbrowser-reviewers,dao

https://hg.mozilla.org/mozilla-central/rev/da2dd0daaa35

Status: NEW → RESOLVED
Closed: 2 months ago
status-firefox130: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 130 Branch
Flags: qe-verify+
Attached video video showing the issue —

I was able to reproduce the issue on Firefox 128.0.3, using Windows 11, while following the steps described in Comment 0.
Verified on Firefox 130.0b2, using Windows 11, macOS 14.4 and Ubuntu 22.04, and the favicon is still not not reseting when changing tab to about:newtab.

If I navigate to about:blank, no favicon is displayed, but this was also the case before the fix.
@Tim, could you please take a look at the attached video and let me know if I am missing something? Thank you in advance!

Flags: needinfo?(timw-bugzilla)

Hi Bianca,

Yes, I am seeing similar behavior on Firefox 130.0b5 on Ubuntu 22.04.

Strange: If I put a debugger breakpoint at the new code here, follow the steps in Comment 0, and then resume execution after it pauses, the favicon changes to the correct Firefox icon.

Flags: needinfo?(timw-bugzilla)
Flags: needinfo?(tgiles)

(In reply to Tim Williams from comment #22)

Hi Bianca,

Yes, I am seeing similar behavior on Firefox 130.0b5 on Ubuntu 22.04.

Strange: If I put a debugger breakpoint at the new code here, follow the steps in Comment 0, and then resume execution after it pauses, the favicon changes to the correct Firefox icon.

Thank you for the answer! In this case, should we reopen this bug as it is still reproducing while following the steps from Comment 0?

Flags: needinfo?(timw-bugzilla)

(In reply to Bianca Hidecuti, Desktop Test Engineering [:bhidecuti] from comment #23)

Thank you for the answer! In this case, should we reopen this bug as it is still reproducing while following the steps from Comment 0?

Of course. I will reach out to Tim Giles, as I am unsure about the next steps.

Hey Tim,

It seems this patch is not displaying the correct behavior after all. I mentioned to Bianca in Comment 22, that if I put a debugger breakpoint at the new code and follow the steps to reproduce the bug, the patch works and the favicon is updated to the nightly icon-- but just browsing from google.com to about:newtab shows the google favicon in the tab still. I am stumped as to why having the breakpoint would change the behavior.

Any ideas on this?

-Tim

Flags: needinfo?(timw-bugzilla) → needinfo?(tgiles)

Went ahead and reopened the bug since the bug is still reproducible, see Comment #21.

It seems this patch is not displaying the correct behavior after all. I mentioned to Bianca in Comment 22, that if I put a debugger breakpoint at the new code and follow the steps to reproduce the bug, the patch works and the favicon is updated to the nightly icon-- but just browsing from google.com to about:newtab shows the google favicon in the tab still. I am stumped as to why having the breakpoint would change the behavior.

We're most likely now dealing with a race condition, or that something else changed in the code that impacts this same path. By stopping on the breakpoint, most likely this is giving a thread enough time to complete the "update favicon" task (don't remember what the function is called in the code) and so the issue is fixed when there's enough of a delay. The fix is not to add an arbitrary delay here though, we don't want to introduce performance regressions and especially not in tabbrowser which I believe is on a critical path.

During my own testing, I'm noticing that even if I throw a debugger statement in the if (!isReload && !aWebProgress.isLoadingDocument) function that was added as part of this patch, that debugger statement is never executed. This is because aWebProgress.isLoadingDocument is almost always true under "normal" circumstances (i.e. when not trying to debug the onLocationChange function).

I'm gonna be honest, this is a strange one. I've done some more digging and, while I don't have a solution, I do have more information.

This seems to be due to the fact that opening about:blank, whether as the new tab option or by manually entering the URL in the search bar, does not cause any DOMLinkAdded events to fire. The onLinkEvent handler of LinkHandlerChild is what eventually causes the icon to switch from the Google favicon back to the local Nightly favicon if you load about:home. This is due to the LinkHandlerChild calling iconLoader.addIconFromLink and since the local Nightly icon is not a rich icon, we set this.seenTabIcon = true, which is what allows us to load the local Nightly icon again if we come back to about:home in the same tab. However, I don't know how to special case about:blank when it is set as the new tab behavior, without introducing issues to about:blank in general.

:mconley, I'm a bit out of my depth at this point, any chance you have some ideas on how to get the onLinkEvent handler to fire on about:blank only when it is set as the new tab page? See the previous paragraph for more context. I don't think we want to bypass the onLinkEvent handler as that has some optimizations already. It seems like the particular DOMLinkAdded event is something is dynamically adding <link rel="icon" type="image/png" href="chrome://branding/content/icon32.png"> to about:home, so I'm guessing we need to do something like that for our particular case. I'm not entirely sure where this DOMLinkAdded event is being fired from, guess it's somewhere in the C++ layer (probably in onPageShow in Document.cpp but I haven't verified that) and I'm not sure how the event is getting constructed with the right properties:

DOMLinkAdded { target: link, isTrusted: true, srcElement: link, currentTarget: WindowRoot, eventPhase: 2, bubbles: true, cancelable: false, returnValue: true, defaultPrevented: false, composed: false, … }
bubbles: true
cancelBubble: false
cancelable: false
composed: false
composedTarget: <link rel="icon" type="image/png" href="chrome://branding/content/icon32.png">
currentTarget: WindowRoot { ownerGlobal: Window }
defaultPrevented: false
defaultPreventedByChrome: false
defaultPreventedByContent: false
eventPhase: 2
explicitOriginalTarget: <link rel="icon" type="image/png" href="chrome://branding/content/icon32.png">
isReplyEventFromRemoteContent: false
isSynthesized: false
isTrusted: true
isWaitingReplyFromRemoteContent: false
multipleActionsPrevented: false
originalTarget: <link rel="icon" type="image/png" href="chrome://branding/content/icon32.png">
returnValue: true
srcElement: <link rel="icon" type="image/png" href="chrome://branding/content/icon32.png">
target: <link rel="icon" type="image/png" href="chrome://branding/content/icon32.png">
timeStamp: 14
type: "DOMLinkAdded"
<get isTrusted()>: function isTrusted()
<prototype>: EventPrototype { composedPath: composedPath(), stopPropagation: stopPropagation(), stopImmediatePropagation: stopImmediatePropagation(), … }

tl;dr: current hypothesis is that since there's no DOM links being added in about:blank, the onLinkEvent handler of LinkHandlerChild.sys.mjs is never called and the icon loader doesn't get a chance to set the favicon of the new about:blank tab.

Status: RESOLVED → REOPENED
Flags: needinfo?(tgiles) → needinfo?(mconley)
Resolution: FIXED → ---

Thank you for the additional context and all the digging you've done. Looking forward to learning more as this thread progresses :)

Hi Tim and Tim! Thanks for poking at this. Wow, tabbrowser's location change and state change stuff is pretty complicated.

I think the DOMLinkAdded event and the LinkHandlerChild stuff might be a bit of a red herring. about:blank is a very special kind of minimal document - it's not meant to have things like <link> tags. So instead of trying to modify about:blank to behave like a normal webpage, what I suggest is we continue to hack tabbrowser.js for the about:blank (and about:newtab as about:blank) use case.

What I suggest Tim (Williams) is that we move what you've added here outside of onLocationChange. Instead of doing this kind of thing on location change, let's do it in onStateChange once the state has reached STATE_STOP (which means that the tab has finished loading).

Something like this around here:

        if (
          !this.mBrowser.mIconURL &&
          !ignoreBlank &&
          !(originalLocation.spec in FAVICON_DEFAULTS)
        ) {
          this.mTab.removeAttribute("image");
        } else {
          // This is a no-op unless this.mBrowser._documentURI is in
          // FAVICON_DEFAULTS.
          gBrowser.setDefaultIcon(this.mTab, this.mBrowser._documentURI);
        }

So this changes things so we make the determination about setting the default icon only after the load completes. Does that work for you?

Flags: needinfo?(mconley) → needinfo?(tgiles)

Hi Mike! Thank you for that. I implemented the changes you outlined in your comment and I am now seeing the desired outcome (Firefox icon in newtab) when following the steps from Comment 0 .

Tim, I reopened https://phabricator.services.mozilla.com/D216357 and pushed the patch with the new changes there.

Cheers,
Tim

Pushed by tgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d49cd728c29
favicon not reseting when changing tab to about:newtab with newtab to Blank Page.r=tgiles,tabbrowser-reviewers,dao

https://hg.mozilla.org/mozilla-central/rev/7d49cd728c29

Status: REOPENED → RESOLVED
Closed: 2 months ago → 15 days ago
status-firefox131: --- → fixed
Resolution: --- → FIXED

Verified as fixed on Firefox Nightly 131.0a1 (2024-08-30), using macOS 14.6, Windows 11 and Ubuntu 22.04.
The favicon is now reseting when changing tab to about:newtab while following the steps from Comment 0.

@Tim, can we update the tracking flag for firefox130 as the bug is not fixed there? Thank you in advance!

status-firefox131: fixed → verified
Flags: needinfo?(timw-bugzilla)
status-firefox130: fixed → wontfix
Flags: needinfo?(timw-bugzilla)
Flags: needinfo?(tgiles)

Bianca, thanks for your help in resurfacing this bug and helping to get it fixed. It looks like Tim has updated the tracking flag for firefox130. Thanks, Tim :)

Cheers!

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: