Use channel->ascynOpen2 in netwerk/base

RESOLVED FIXED in Firefox 43

Status

()

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

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Blocks: 1182535
(Assignee)

Comment 1

3 years ago
Created attachment 8653795 [details] [diff] [review]
bug_1199491_nsincrementaldownload.patch
Attachment #8653795 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
Summary: Use channel->ascynOpen2 in netwerk/base/nsIncrementalDownload.cpp → Use channel->ascynOpen2 in netwerk/base
(Assignee)

Comment 2

3 years ago
Created attachment 8653799 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base.patch

Jonas, that reminds me, can't we just remove the 'enforceSecurity' flag completely and just rely on 'securityMode'? I think enforceSecurity is completely unnecessary. Besides, I think in the case of nsIURIChecker not even accurate, because the channel might have not been openend until now, so SetEnforceSecurity would probably not even have been set in the loadInfo.
Attachment #8653795 - Attachment is obsolete: true
Attachment #8653795 - Flags: review?(jonas)
Attachment #8653799 - Flags: review?(jonas)
Comment on attachment 8653799 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base.patch

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

::: netwerk/base/nsURIChecker.cpp
@@ +188,5 @@
> +        rv = mChannel->AsyncOpen2(this);
> +    }
> +    else {
> +        rv = mChannel->AsyncOpen(this, nullptr);
> +    }

This seems strange. When would GetEncorceSecurity() ever return true before we've called AsyncOpen/AsyncOpen2?

Also, since this uses a nullprincipal as the loadingprincipal, won't AsyncOpen2 always fail?

I suspect we need to find a more correct principal to use, and then simply use AsyncOpen2. Quite likely that correct principal is the system principal.
Attachment #8653799 - Flags: review?(jonas) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8656300 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base.patch

Hey Steve, since you reviewed [1] I was wondering if you are fine with using the systemPrincipal as the loadingPrincipal in nsURIChecker.cpp. I think it is the system which actually performs the load. What do you think?

[1] http://hg.mozilla.org/mozilla-central/rev/b75f375f7cb5
Attachment #8656300 - Flags: review?(sworkman)
(Assignee)

Updated

3 years ago
Attachment #8653799 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Comment on attachment 8656300 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base.patch

Jason, Steve will be on PTO starting tomorrow and he will not get to that quickly.

Question is only about nsURIChecker. Is the system performing the load? If so, I assume using the systemPrincipal as the loadingPrincipal is the right thing to do. Or do we have to change some signatures and pass along a loadingPrincipal??
Attachment #8656300 - Flags: review?(sworkman) → review?(jduell.mcbugs)
Is the urichecker code used anywhere? I can't find any code that uses it anywhere in the tree.

http://mxr.mozilla.org/mozilla-central/search?string=urichecker
(Assignee)

Comment 7

3 years ago
Created attachment 8656312 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base_1.patch
Attachment #8656300 - Attachment is obsolete: true
Attachment #8656300 - Flags: review?(jduell.mcbugs)
Attachment #8656312 - Flags: review?(jonas)
(Assignee)

Comment 8

3 years ago
Created attachment 8656313 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base_2.patch

(In reply to Jonas Sicking (:sicking) from comment #6)
> Is the urichecker code used anywhere? I can't find any code that uses it
> anywhere in the tree.

Well, we can definitely forward the loadInfo of the channel from which we grab the uri. It's definitely more accurate than using the systemPrincipal. But we should also have Jason have a look at it.
Attachment #8656313 - Flags: review?(jonas)
Attachment #8656313 - Flags: review?(jduell.mcbugs)
Comment on attachment 8656313 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base_2.patch

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

::: netwerk/base/nsURIChecker.cpp
@@ +107,5 @@
>              // parameters that may have been set on lastChannel??
>  
>              if (NS_SUCCEEDED(rv)) {
> +                nsCOMPtr<nsILoadInfo> loadInfo = lastChannel->GetLoadInfo();
> +                rv = Init(uri, loadInfo);

This will create a new channel which uses the same loadinfo object. I don't think we want this. If anything we'll want to use a copy of the loadinfo.

But in any case, this just does some rechecking if the original request resulted in a 404. So we'll only get here if someone else has already called Init(uri) previously.

@@ +134,5 @@
>  // nsIURIChecker methods:
>  //-----------------------------------------------------------------------------
>  
>  NS_IMETHODIMP
> +nsURIChecker::Init(nsIURI *aURI, nsILoadInfo* aLoadInfo)

This will break any existing callers of Init. But again, I can't find any in the tree.
Attachment #8656313 - Flags: review?(jonas) → review-
(Assignee)

Comment 10

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #9)
> Comment on attachment 8656313 [details] [diff] [review]
> bug_1199491_asyncOpen2_netwerk_base_2.patch
> 
> Review of attachment 8656313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsURIChecker.cpp
> @@ +107,5 @@
> >              // parameters that may have been set on lastChannel??
> >  
> >              if (NS_SUCCEEDED(rv)) {
> > +                nsCOMPtr<nsILoadInfo> loadInfo = lastChannel->GetLoadInfo();
> > +                rv = Init(uri, loadInfo);
> 
> This will create a new channel which uses the same loadinfo object. I don't
> think we want this. If anything we'll want to use a copy of the loadinfo.
> 
> But in any case, this just does some rechecking if the original request
> resulted in a 404. So we'll only get here if someone else has already called
> Init(uri) previously.

So then maybe the right thing to do is actually use the systemprincipal. Let's wait for Jason's feedback and then I will update the patch accordingly.
Comment on attachment 8656313 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base_2.patch

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

Yet more corners of necko I barely knew existed :)

So there are 16 addons which use nsIURIChecker:

  https://mxr.mozilla.org/addons/search?string=nsIURIChecker

It's not good that we break all of their init() calls.

It does seem to me that URIChecker is a system request, so I don't object to using that (especially if it doesn't break Init).  But I'm not really an expert on principals (but you guys are, so I'll trust you).
Attachment #8656313 - Flags: review?(jduell.mcbugs) → review-
(Assignee)

Comment 12

3 years ago
Created attachment 8659977 [details] [diff] [review]
bug_1199491_asyncOpen2_netwerk_base_2.patch

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #11)
> It does seem to me that URIChecker is a system request, so I don't object to
> using that (especially if it doesn't break Init).  But I'm not really an
> expert on principals (but you guys are, so I'll trust you).

Alright then, thanks for clarification everyone. In that case we are just replacing the nullPrincipal with the systemPrincipal.
Attachment #8656313 - Attachment is obsolete: true
Attachment #8659977 - Flags: review?(jonas)
https://hg.mozilla.org/mozilla-central/rev/274e1235193e
https://hg.mozilla.org/mozilla-central/rev/13082cd98557
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.