1449262 - Favicon requests cause proxy lookups with an unknown tabId

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, enhancement, P3)

Description

[Dave Townsend [:mossop]]

When the browser requests the favicon for a page it causes a proxy request through the webextension proxy API but the tabId for the request is unknown. Ideally we'd be able to tie this request to a specific tab.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Bug 1449262: Associate favicon requests with the nsILoadContext of the tab the favicon is needed for.

Review commit: https://reviewboard.mozilla.org/r/233808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233808/

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8965116 [details]
Bug 1449262: Associate favicon requests with the nsILoadContext of the tab the favicon is needed for.

This is my first attempt here. It seems to work in most cases, some are still showing up unassociated that I haven't tracked down. Basically it just gives the channel the nsILoadContext from the tab. This has the added benefit that we no longer need to tell the service whether to load the icon privately or not, it gets that from the nsILoadContext.

How does this look as an approach?

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8965116 [details]
Bug 1449262: Associate favicon requests with the nsILoadContext of the tab the favicon is needed for.

https://reviewboard.mozilla.org/r/233808/#review241292

I see a few issues to figure out:
1. can we generate a fake private loadContext in xpcshell? Asking because some favicons xpcshell tests will need that. I assume Cc["@mozilla.org/privateloadcontext;1"].createInstance(Ci.nsILoadContext) will do?
2. is LoadContext thread-safe? AsyncFetchAndSetIconForPage acts on both threads, if load context is not thread safe, we should ensure it gets addrefed, used and destroyed on the same thread.
3. Off-hand doesn't look like this may generate cycles, but it's hard to tell

Passing the loadcontext to AsyncFetchAndSetIconForPage (and to the aPI) sounds like a good idea, but I wonder if at that point we could just immediately extract the info we need from it and store that into primitive properties, then we wouldn't need to addref it and risk thread-safety problems.

Comment 4

[Dave Townsend [:mossop]]

(In reply to Marco Bonardo [::mak] from comment #3)
> Comment on attachment 8965116 [details]
> Bug 1449262: Associate favicon requests with the nsILoadContext of the tab
> the favicon is needed for.
> 
> https://reviewboard.mozilla.org/r/233808/#review241292
> 
> I see a few issues to figure out:
> 1. can we generate a fake private loadContext in xpcshell? Asking because
> some favicons xpcshell tests will need that. I assume
> Cc["@mozilla.org/privateloadcontext;1"].createInstance(Ci.nsILoadContext)
> will do?

Yeah seems like there are ways to generate non-private and private load contexts for testing so that's nice.

> 2. is LoadContext thread-safe? AsyncFetchAndSetIconForPage acts on both
> threads, if load context is not thread safe, we should ensure it gets
> addrefed, used and destroyed on the same thread.

Unfortunately not. The normal load context is just the docshell and I don't think that is thread safe :(
I'll look into this and see if there is a way to make it work.

> 3. Off-hand doesn't look like this may generate cycles, but it's hard to tell
> 
> Passing the loadcontext to AsyncFetchAndSetIconForPage (and to the aPI)
> sounds like a good idea, but I wonder if at that point we could just
> immediately extract the info we need from it and store that into primitive
> properties, then we wouldn't need to addref it and risk thread-safety
> problems.

Might be doable. Right now the WebExtension code relies on a load context to find the browser responsible for the load. I might be able to change that though.

Thanks.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8965116 [details]
Bug 1449262: Associate favicon requests with the nsILoadContext of the tab the favicon is needed for.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233808/diff/1-2/

Comment 6

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #4)
> > 2. is LoadContext thread-safe? AsyncFetchAndSetIconForPage acts on both
> > threads, if load context is not thread safe, we should ensure it gets
> > addrefed, used and destroyed on the same thread.
> 
> Unfortunately not. The normal load context is just the docshell and I don't
> think that is thread safe :(
> I'll look into this and see if there is a way to make it work.

Ok, I've looked at this and while the context is not thread safe we only ever need to access it from the main thread (aside from addref/release), so I've switched to holding it with a nsMainThreadPtr to solve this.

> 
> > 3. Off-hand doesn't look like this may generate cycles, but it's hard to tell
> > 
> > Passing the loadcontext to AsyncFetchAndSetIconForPage (and to the aPI)
> > sounds like a good idea, but I wonder if at that point we could just
> > immediately extract the info we need from it and store that into primitive
> > properties, then we wouldn't need to addref it and risk thread-safety
> > problems.
> 
> Might be doable. Right now the WebExtension code relies on a load context to
> find the browser responsible for the load. I might be able to change that
> though.

Kris, do you see any other alternative to what I'm doing? Maybe I could somehow correlate to the outerwindow ID or something from ChannelWrapper::GetBrowserElement ?

Comment 7

[Kris Maglione [:kmag]]

I think this is probably a good solution, as long as we make sure to only touch the load context on the main thread, and null it out when the request is finished, to avoid any possible reference cycles.

I suppose we *could* theoretically store the cached outer window ID, but that might be a bit dodgy. We'd have to iterate over all browser windows to map that to an actual tab, and it can change throughout the load cycle. Storing the TabParent ID and mapping that to a <browser> when we need it might be better, but I don't think that's really any better than just storing a reference to the load context.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8965116 [details]
Bug 1449262: Associate favicon requests with the nsILoadContext of the tab the favicon is needed for.

Ok for me to go ahead and start fixing the tests and probably adding some more in that case?

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8965116 [details]
Bug 1449262: Associate favicon requests with the nsILoadContext of the tab the favicon is needed for.

https://reviewboard.mozilla.org/r/233808/#review242154

Yes, it looks ok to me

::: browser/components/places/PlacesUIUtils.jsm:186
(Diff revision 2)
> -      ? PlacesUtils.favicons.FAVICON_LOAD_PRIVATE
> -      : PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE;
>      let callback = this._makeCompletionCallback(win, innerWindowID);
>      let request = PlacesUtils.favicons.setAndFetchFaviconForPage(currentURI, uri, false,
> -                                                                 loadType, callback, principal,
> -                                                                 requestContextID);
> +                                                                 callback, principal,
> +                                                                 requestContextID, loadContext);

nit: could probably indent on its own line

::: toolkit/components/places/mozIAsyncFavicons.idl:67
(Diff revision 2)
>      in boolean aForceReload,
> -    in unsigned long aFaviconLoadType,
>      [optional] in nsIFaviconDataCallback aCallback,
>      [optional] in nsIPrincipal aLoadingPrincipal,
> -    [optional] in unsigned long long aRequestContextID);
> +    [optional] in unsigned long long aRequestContextID,
> +    [optional] in nsILoadContext aLoadContext);

should be documented

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8965116 [details]
Bug 1449262: Associate favicon requests with the nsILoadContext of the tab the favicon is needed for.

You should probably still null loadContext once onStopRequest or when we don't need it anymore.

Comment 11

[Dave Townsend [:mossop]]

Fixed by bug 1453751