1257930 - Update NetUtil.asyncFetch() to use asyncOpen2()

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1257930_update_netutil_asyncfetch.patch

proof of concept patch!

I am not really sure if we should add asyncFetch2() and update callsites or try handle everything within asyncFetch().

For example, we could check if the channel has a loadInfo and the proper security flags, then call asyncOpen2() on it. We could log deprecation warnings and make sure to update all callsites within Gecko.

Either way, we have to wait till Bug 1257339 brings back the right API on NetUtil.newChannel before we can move forward here.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1257930_update_netutil_asyncfetch_asyncopen.patch

This patch throws an exception if we can't call asyncOpen2(). We should remove that exception before landing, but it will help us make sure we have updated all the callsites within Gecko:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2cefc749bb9

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1257930_update_netutil_asyncfetch_asyncopen.patch

Jonas, browser_favicon_setAndFetchFaviconForPage.js would fail, because we don't support TYPE_IMAGE within the contentsecuritymanager yet. As a tmp workaround we could pass SEC_NORMAL which would allow us to use asyncOpen2() within asyncFetch(). Otherwise we would have to make this bug being blocked on the imgLoader conversion as well.

I lean towards falling back to SEC_NORMAL for now so we can use asyncOpen() within asyncFetch().

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8741442 [details] [diff] [review]
bug_1257930_update_netutil_asyncfetch_asyncopen.patch

Review of attachment 8741442 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below caveat.

::: toolkit/components/places/tests/browser/browser_favicon_setAndFetchFaviconForPage.js
@@ +33,5 @@
>      NetUtil.asyncFetch({
>        uri: favIconLocation,
>        loadUsingSystemPrincipal: true,
> +      // XXXckerschb: remove securityFlags once imageLoader uses asyncOpen2()
> +      securityFlags: Ci.nsILoadInfo.SEC_NORMAL,

I still don't think this change is the better way to go. But as long as you make sure we fix it in a followup I can live with it.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> I still don't think this change is the better way to go. But as long as you
> make sure we fix it in a followup I can live with it.

I will make sure that happens; thanks!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22521e3f311e

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/22521e3f311e