Update NetUtil.asyncFetch() to use asyncOpen2()

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Blocks: 1182535
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8732304 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Depends on: 1257339
(Assignee)

Updated

2 years ago
Whiteboard: [domsecurity-active]
(Assignee)

Comment 2

2 years ago
Created attachment 8740865 [details] [diff] [review]
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
Attachment #8732304 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
Created attachment 8741442 [details] [diff] [review]
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().
Attachment #8740865 - Attachment is obsolete: true
Attachment #8741442 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 5

2 years ago
(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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22521e3f311e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

2 years ago
Depends on: 1268595
(Assignee)

Updated

2 years ago
Depends on: 1269261
You need to log in before you can comment on or make changes to this bug.