Error: When cancelling a request for chrome://branding/content/icon32.png because the chrome window went away

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Places
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: Yaron, Assigned: Gijs)

Tracking

({regression})

47 Branch
mozilla51
regression
Points:
---

Firefox Tracking Flags

(firefox48- wontfix, firefox49- wontfix, firefox50- wontfix, firefox51+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

Close a tab.


Actual results:

You often get the following error in the Error Console:

When cancelling a request for chrome://branding/content/icon32.png because the chrome window went away, it was already canceled!
resource:///modules/PlacesUIUtils.jsm
Line: 109

Or:

When cancelling a request for https://assets-cdn.github.com/favicon.ico because the chrome window went away, it was already canceled!
resource:///modules/PlacesUIUtils.jsm
Line: 109


The URL in the error message changes according to the closed tab.

Updated

2 years ago
Component: Untriaged → Places
Product: Firefox → Toolkit

Comment 1

a year ago
Steps to reproduce
1. Start Nightly
2. Go to about:home
3. Open the Browser Console (Ctrl+Shift+J)
4. Wait one minute
5. Reload current page or Click Home button
6. You can see error message below

When cancelling a request for chrome://branding/content/icon32.png because the inner window was destroyed or a new favicon was loaded for it, it was already canceled!	PlacesUIUtils.jsm:109
 _cancelRequest	resource:///modules/PlacesUIUtils.jsm:109:7
 removeRequestsForInner/newLoadDataForWindow<	resource:///modules/PlacesUIUtils.jsm:121:11
 filter	self-hosted
 removeRequestsForInner	resource:///modules/PlacesUIUtils.jsm:118:34
 ensureInitialized/<	resource:///modules/PlacesUIUtils.jsm:183:7

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=ec97adea519ba55ac05a2af28534e0594b97f80e&tochange=3ff4077f9e4bf923322c95fb7226bc0e9090f898

I am not authorized to access bug 1255270
Status: UNCONFIRMED → NEW
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
Ever confirmed: true

Comment 2

a year ago
Gijs, when you are back from PTO, could you check this issue, please.
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: regression
Whiteboard: [regressed by bug 1255270]
(Assignee)

Comment 3

a year ago
(In reply to Loic from comment #2)
> Gijs, when you are back from PTO, could you check this issue, please.

I can look at this on Monday. What would be helpful if someone can clarify what we're looking at, because the message in comment #0 is different from the one in comment #1.

Comment 4

a year ago
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #3)

> I can look at this on Monday. What would be helpful if someone can clarify
> what we're looking at, because the message in comment #0 is different from
> the one in comment #1.

This reason is Firefox version. (47 and Nightly)
(Assignee)

Comment 5

a year ago
(In reply to magicp from comment #4)
> (In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #3)
> 
> > I can look at this on Monday. What would be helpful if someone can clarify
> > what we're looking at, because the message in comment #0 is different from
> > the one in comment #1.
> 
> This reason is Firefox version. (47 and Nightly)

So do you have steps that reproduce the same problem for the github icon (rather than the branding one) from comment #0 on nightly? At this stage I'm less interested in 47 which is already old (as 48 has been released). I was aware of this issue on 47. More concerned if the same thing happens on 48, especially if with non-chrome icons.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(magicp.jp)

Comment 6

a year ago
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #5)

> So do you have steps that reproduce the same problem for the github icon
> (rather than the branding one) from comment #0 on nightly? At this stage I'm
> less interested in 47 which is already old (as 48 has been released). I was
> aware of this issue on 47. More concerned if the same thing happens on 48,
> especially if with non-chrome icons.

Reproduce on 47 to Nightly with same steps that I wrote.

Error message on 48 below
When cancelling a request for chrome://branding/content/icon32.png because the inner window was destroyed or a new favicon was loaded for it, it was already canceled! PlacesUIUtils.jsm:109
 _cancelRequest() PlacesUIUtils.jsm:109
 removeRequestsForInner/newLoadDataForWindow<()PlacesUIUtils.jsm:121
 filter() self-hosted
 removeRequestsForInner() PlacesUIUtils.jsm:118
 observe() PlacesUIUtils.jsm:94
Flags: needinfo?(magicp.jp)
(Assignee)

Comment 7

a year ago
(In reply to magicp from comment #6)
> (In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #5)
> 
> > So do you have steps that reproduce the same problem for the github icon
> > (rather than the branding one) from comment #0 on nightly? At this stage I'm
> > less interested in 47 which is already old (as 48 has been released). I was
> > aware of this issue on 47. More concerned if the same thing happens on 48,
> > especially if with non-chrome icons.
> 
> Reproduce on 47 to Nightly with same steps that I wrote.
> 
> Error message on 48 below
> When cancelling a request for chrome://branding/content/icon32.png because
> the inner window was destroyed or a new favicon was loaded for it, it was
> already canceled! PlacesUIUtils.jsm:109
>  _cancelRequest() PlacesUIUtils.jsm:109
>  removeRequestsForInner/newLoadDataForWindow<()PlacesUIUtils.jsm:121
>  filter() self-hosted
>  removeRequestsForInner() PlacesUIUtils.jsm:118
>  observe() PlacesUIUtils.jsm:94

I'm afraid I don't understand. This still references chrome://branding/content/icon32.png . What I'm asking is this: do you know of a way to reproduce this that produces such error messages for http/https icons (rather than chrome:// ones), on 48-51?
Flags: needinfo?(magicp.jp)

Comment 8

a year ago
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #7)

> I'm afraid I don't understand. This still references
> chrome://branding/content/icon32.png . What I'm asking is this: do you know
> of a way to reproduce this that produces such error messages for http/https
> icons (rather than chrome:// ones), on 48-51?

I understand non-chrome icons mean. But I don't know non-chrome steps.
Flags: needinfo?(magicp.jp)
(Reporter)

Comment 9

a year ago
I find the exact STR somewhat illusive at this point.

Anyway -
Open a private window.
Open and close several tabs.

After a few minutes I get the error(s) in the Error Console (I'm still using the good old console).
* I'm referring to http/https icons. *

Thank you all.
(Reporter)

Comment 10

a year ago
I've just closed this page and got multiple errors.
https://s3.postimg.org/fv92ilqzn/image.png
(Assignee)

Comment 11

a year ago
(In reply to Yaron from comment #10)
> I've just closed this page and got multiple errors.
> https://s3.postimg.org/fv92ilqzn/image.png

Yes, but you're using 47 and not 48 (because the error console was removed for 48, so this screenshot can't be from 48), but 48 is current release (you should update!). The question is about 48 and later, and non-chrome icons. Anyway, I think I'll just write the obvious patch here and deal with this that way...

48 has been released and a stray error in the browser console isn't enough of a reason to ship a dot-release (and getting rid of it is non-trivial). If the error is restricted to chrome:// icons on 48, I don't think there are any negative effects other than the message showing up, and so this doesn't warrant fixing on 48, and so I'm marking this wontfix for 48.
Blocks: 1255270
status-firefox48: affected → wontfix
Whiteboard: [regressed by bug 1255270]
(Reporter)

Comment 12

a year ago
Hello Gijs,

I'm using 48.01! (unbranded edition).
The error console was removed from the UI but I still use toJavaConsole(). :)

Thanks.

Comment 13

a year ago
I found non-chrome STR! Please try below.

Before STR, please set the following settings at "about:preferences#privacy"
  - History Firefox will: "Use custom settings for history"
  - Check-off "Remember my browsing and download history"

1. Start Firefox 48
2. Open a new tab
3. Go to any sites (e.g. https://bugzilla.mozilla.org)
4. Wait one minute
5. Close current tab
6. Check the error in the Browser Console
(Assignee)

Comment 14

a year ago
I'll try to look at this tomorrow, then, I'm away most of today.
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 15

a year ago
oncommand="toJavaScriptConsole();"
not a recent regression
tracking-firefox48: ? → -
(Reporter)

Comment 17

a year ago
(In reply to magicp from comment #13)
> I found non-chrome STR! Please try below.
> 
> Before STR, please set the following settings at "about:preferences#privacy"
>   - History Firefox will: "Use custom settings for history"
>   - Check-off "Remember my browsing and download history"
> 
> 1. Start Firefox 48
> 2. Open a new tab
> 3. Go to any sites (e.g. https://bugzilla.mozilla.org)
> 4. Wait one minute
> 5. Close current tab
> 6. Check the error in the Browser Console

I'm not sure "Remember my browsing and download history" is the culprit.
I sometimes get the error with this setting checked, and the error might *not* appear when the setting is unchecked.

Is it consistent on your machine?

Thanks.

Comment 18

a year ago
Created attachment 8781295 [details]
regression-environment-by-version-by-locale.png

(In reply to Yaron from comment #17)

> Is it consistent on your machine?
Yes, it is. Using a new profile by version, by locale for regression.
(Reporter)

Comment 19

a year ago
Thank you magicp. I appreciate it.
(Assignee)

Comment 20

a year ago
FWIW, this error has no actual bad effects, so I don't think it needs to track anything or be uplifted, but it's up to relman.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment on attachment 8782068 [details]
Bug 1279650 - kill off useless warning for timed out favicon requests that have already been cancelled when the window closes,

https://reviewboard.mozilla.org/r/72326/#review69944

Thanks Gijs! r=me with comment addressed.

::: browser/components/places/PlacesUIUtils.jsm:147
(Diff revision 1)
>        }
>      }
>      gFaviconLoadDataMap.delete(win);
>    },
>  
> +  _spliceLoadDataFromWindowMap(win, filterData) {

I'd rename this to `_removeLoadDataForWindow`...
You can destructure the `filterParams`, so you get
`_removeDataForWindow(win, { innerWindowID, uri, callback }) {`
Attachment #8782068 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 23

a year ago
mozreview-review
Comment on attachment 8782068 [details]
Bug 1279650 - kill off useless warning for timed out favicon requests that have already been cancelled when the window closes,

https://reviewboard.mozilla.org/r/72326/#review70260

::: browser/components/places/PlacesUIUtils.jsm:147
(Diff revision 1)
>        }
>      }
>      gFaviconLoadDataMap.delete(win);
>    },
>  
> +  _spliceLoadDataFromWindowMap(win, filterData) {

Renamed to `_removeLoadDataFromWindowMap` because "for window" is confusing, as we have a separate method that removes *all* the load data for a given inner (content) window. Instead, the window parameter here is simply an argument we happen to need to look up the load data to remove in the chrome window (not inner/content window!) based map. I'll also add a jsdoc comment.
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #23)
> Renamed to `_removeLoadDataFromWindowMap` because "for window" is confusing,
> as we have a separate method that removes *all* the load data for a given
> inner (content) window. Instead, the window parameter here is simply an
> argument we happen to need to look up the load data to remove in the chrome
> window (not inner/content window!) based map. I'll also add a jsdoc comment.

Me gusta.
Comment hidden (mozreview-request)

Comment 26

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fbc71dcb173a
kill off useless warning for timed out favicon requests that have already been cancelled when the window closes, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/fbc71dcb173a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Reporter)

Comment 28

a year ago
Thank you Gijs and all.
Tracking 51+ for this regression.
tracking-firefox51: ? → +
There is no significant user impact but message showing up and is not a recent regression. Un-track 49 and mark 49 as fix-optional.
status-firefox49: affected → fix-optional
tracking-firefox49: ? → -
Hey Gijs, should we uplift this fix to Beta50?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 32

a year ago
(In reply to Ritu Kothari (:ritu) from comment #31)
> Hey Gijs, should we uplift this fix to Beta50?

The only problem here is the warnings in the console. The change is not entirely trivial, though still probably reasonably low risk. I just personally wouldn't bother just to get rid of some warnings on the console.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(rkothari)
(In reply to :Gijs Kruitbosch from comment #32)
> (In reply to Ritu Kothari (:ritu) from comment #31)
> > Hey Gijs, should we uplift this fix to Beta50?
> 
> The only problem here is the warnings in the console. The change is not
> entirely trivial, though still probably reasonably low risk. I just
> personally wouldn't bother just to get rid of some warnings on the console.

Thanks, your assessment makes sense. Wontfix for Fx50.
Flags: needinfo?(rkothari)

Updated

a year ago
status-firefox50: affected → wontfix
tracking-firefox50: ? → -
status-firefox49: fix-optional → wontfix
You need to log in before you can comment on or make changes to this bug.