Closed Bug 1257930 Opened 4 years ago Closed 4 years ago

Update NetUtil.asyncFetch() to use asyncOpen2()

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
Status: NEW → ASSIGNED
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.
Depends on: 1257339
Whiteboard: [domsecurity-active]
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
Attachment #8732304 - Attachment is obsolete: true
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().
Attachment #8740865 - Attachment is obsolete: true
Attachment #8741442 - Flags: review?(jonas)
Summary: Update NetUtil.asyncFetch() to use newChannel2() → Update NetUtil.asyncFetch() to use asyncOpen2()
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.
Attachment #8741442 - Flags: review?(jonas) → review+
(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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/22521e3f311e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1268595
Depends on: 1269261
You need to log in before you can comment on or make changes to this bug.