Closed Bug 1192948 Opened 5 years ago Closed 5 years ago

Use channel->ascynOpen2 in uriloader/prefetch

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
I suppose we can update these callsites at the same time:
/uriloader/base/nsURILoader.cpp
/uriloader/prefetch/nsOfflineCacheUpdate.cpp
/uriloader/prefetch/nsPrefetchService.cpp
Assignee: nobody → mozilla
Blocks: 1182535
In fact, we should wait with updating asyncOpen() to asyncOpen(2) within nsURILoader, because that is used from within docshell:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10921

But we should be ready to do docshell soonish I suppose!
Attachment #8645926 - Flags: review?(jonas)
Comment on attachment 8645926 [details] [diff] [review]
bug_1192948_asyncopen2_prefetch.patch

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

Are there really no existing security checks?!? Not even CheckLoadURI?

I don't feel quite comfortable reviewing this. Could you ask someone that knows this code better? (I don't know who. Hopefully hg log can tell you)
Attachment #8645926 - Flags: review?(jonas)
Comment on attachment 8645926 [details] [diff] [review]
bug_1192948_asyncopen2_prefetch.patch

(In reply to Jonas Sicking (:sicking) from comment #3)
> Are there really no existing security checks?!? Not even CheckLoadURI?

I couldn't find any.
 
> I don't feel quite comfortable reviewing this. Could you ask someone that
> knows this code better? (I don't know who. Hopefully hg log can tell you)

Olli, do you feel you wanna review this? Might be a good start, because converting docshell is the next big thing we are going to convert. It would be great if you could help us reviewing.
Attachment #8645926 - Flags: review?(bugs)
Comment on attachment 8645926 [details] [diff] [review]
bug_1192948_asyncopen2_prefetch.patch

(I think we should pass some sane principal, but that issue isn't about this bug, but about http://hg.mozilla.org/mozilla-central/diff/d4ced32fc77b/uriloader/prefetch/nsOfflineCacheUpdate.cpp)
Attachment #8645926 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load) from comment #5)
> Comment on attachment 8645926 [details] [diff] [review]
> bug_1192948_asyncopen2_prefetch.patch
> 
> (I think we should pass some sane principal, but that issue isn't about this
> bug, but about
> http://hg.mozilla.org/mozilla-central/diff/d4ced32fc77b/uriloader/prefetch/
> nsOfflineCacheUpdate.cpp)

Thanks smaug, I filed Bug 1199295 and made it block the tracking bug [Bug 1182535]. But to sum it up, using the systemPrincipal as the loadingPrincipal does not make things worse as they are right now, but I agree, we could improve things by providing a more accurate loadingPrincipal.
I slightly modified the securityflag which we pass to the loadInfo. Instead of using SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS we now pass SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL. Since the loadingPrincipal is the systemPrincipal, this does not make a real difference, because security checks are bypassed, it only makes a SEMANTICAL difference.
Attachment #8645926 - Attachment is obsolete: true
Attachment #8653557 - Flags: review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/31332b421bf84f1963c4cb1cbd521ad6de403f9c
changeset:  31332b421bf84f1963c4cb1cbd521ad6de403f9c
user:       Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:       Thu Aug 27 19:45:30 2015 -0700
description:
Bug 1192948 - Use channel->ascynOpen2 in uriloader/prefetch (r=smaug)
https://hg.mozilla.org/mozilla-central/rev/31332b421bf8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.