1192948 - Use channel->ascynOpen2 in uriloader/prefetch

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

I suppose we can update these callsites at the same time:
/uriloader/base/nsURILoader.cpp
/uriloader/prefetch/nsOfflineCacheUpdate.cpp
/uriloader/prefetch/nsPrefetchService.cpp

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1192948_asyncopen2_prefetch.patch

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!

Comment 3

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

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)

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1192948_asyncopen2_prefetch.patch

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.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=640a912aea61

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/31332b421bf84f1963c4cb1cbd521ad6de403f9c
changeset:  31332b421bf84f1963c4cb1cbd521ad6de403f9c
user:       Christoph Kerschbaumer <[e-mail]>
date:       Thu Aug 27 19:45:30 2015 -0700
description:
Bug 1192948 - Use channel->ascynOpen2 in uriloader/prefetch (r=smaug)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31332b421bf8