Closed
Bug 1257930
Opened 9 years ago
Closed 9 years ago
Update NetUtil.asyncFetch() to use asyncOpen2()
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 2 obsolete files)
4.66 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 2•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 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
Keywords: checkin-needed
Comment 7•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•