Closed Bug 1691868 Opened 4 years ago Closed 2 years ago

Bookmark menu icons may not be loaded with TYPE_INTERNAL_IMAGE_FAVICON anymore

Categories

(Core :: General, defect)

defect

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: mstange, Unassigned)

Details

I noticed a few broken pieces of code that are trying to use the content policy TYPE_INTERNAL_IMAGE_FAVICON for favicon loads from bookmark menu items, but not succeeding at it:

Freddy, since you are doing a lot principal investigation work these days. Can you take a quick look at this one? In particular, are we using the wrong triggering principal (/content type)?

From a quick glance it seems we would use TYPE_INTERNAL_IMAGE instead of of TYPE_INTERNAL_IMAGE_FAVICON, so that wouldn't be too bad, but the wrong principal could cause to send (not send) cookies.

Flags: needinfo?(fbraun)

There's something with comment 0, that I don't understand. First, you talk about the triggeringPrincipal required for QueryTriggeringPrincipal which in turn gives us the right contentpolicytype. However, the Bug 1319908 you mentioned which should have added the attribute set the loadingPrincipal, not the triggeringPrincipal.

Can you expand on what exactly you see with favicon loads that do not succeed?
Maybe Gecko logging with CSMLog:5 could give us the amount of detail we'd need (should print principals and contentpolicytypes for all outgoing requests, so it will be noisy)

Unfortunately my platform doesn't use the Cocoa Widgets so I can't reproduce locally.

Flags: needinfo?(fbraun)

(In reply to Frederik Braun [:freddy] from comment #2)

There's something with comment 0, that I don't understand. First, you talk about the triggeringPrincipal required for QueryTriggeringPrincipal which in turn gives us the right contentpolicytype. However, the Bug 1319908 you mentioned which should have added the attribute set the loadingPrincipal, not the triggeringPrincipal.

I don't know enough about principals to answer this question, but I did find this changeset which landed after bug 1319908 and made GetContentPolicyTypeForUIImageLoading use the triggering principal instead of the loading principal.

Can you expand on what exactly you see with favicon loads that do not succeed?

I don't see any errors or bad behavior. I only noticed code that doesn't do anything, and which might no longer work as intended, so I wanted to call attention to it.
I've removed the unused mContentType fields from the native menu icon loading code in D104634; maybe there is more code that can be removed.

Maybe Gecko logging with CSMLog:5 could give us the amount of detail we'd need (should print principals and contentpolicytypes for all outgoing requests, so it will be noisy)

I can get a log with that tomorrow, if it's interesting.

This bug here is a case of using our internal APIs incorrectly in our favicon / page info code and not a bug in our implementation of principals. Switching Product & Component based on bug 1407498.

I tried reading nevertheless and I really don't understand bug 1407498 and the changeset you specifically mention.
This seems to be renaming a principal that's being stored in the user interface from loadingPrincipal to triggeringingPrincipal, but I do not see where it's used. Does that also mean we switched which principal to use?

A log would help, sure, but what the patch was trying to do is probably way more useful.
Yoshi you wrote the patch, but I realize this is 3 years ago - maybe you remember nevertheless?

Component: DOM: Security → Page Info Window
Flags: needinfo?(allstars.chh)
Product: Core → Firefox

Sorry, I don't remember too much.
I only recall for images if it's loaded from XUL, it will be considered as a favicon load, for the getting right origin attributes.
like https://hg.mozilla.org/mozilla-central/rev/23473b758d55
But the function doesn't exist anymore, so I don't know how it goes now.

Flags: needinfo?(allstars.chh)

(In reply to Frederik Braun [:freddy] from comment #4)

This bug here is a case of using our internal APIs incorrectly in our favicon / page info code and not a bug in our implementation of principals. Switching Product & Component based on bug 1407498.

I don't really understand this bug, but it seems to be more a platform bug than a bug in the code of the Page Info dialog.

Component: Page Info Window → General
Product: Firefox → Core

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(odvarko)

Markus, is this bug report still valid? Are there any STRs we can try to reproduce it and asses the impact?

Flags: needinfo?(odvarko) → needinfo?(mstange.moz)

I don't remember anything about this bug, sorry. I don't know what the impact of using or not using TYPE_INTERNAL_IMAGE_FAVICON is.

Let's close it.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(mstange.moz)
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.