Closed Bug 1182571 Opened 9 years ago Closed 9 years ago

Use channel->asyncOpen2 in /dom/base/nsXMLHttpRequest.cpp

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: ckerschb, Assigned: sicking)

References

Details

Attachments

(4 files, 5 obsolete files)

Fix nsILoadInfo->GetContentPolicyType API to be less ambigious
15.62 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
Patch to fix
19.75 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Additional fixes
25.52 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Additional fixes to nsILoadInfo patch
3.76 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
nsm volunteered to take on this bug. Even though that's a very rudimentary start, it might give you an idea of what we have to look out for when converting xmlhttpreqeust to use asyncopen2. Thanks for looking into that.
Assignee: mozilla → nsm.nikhil
Summary: Use channel->ascynOpen2 in /dom/base/nsXMLHttpRequest.cpp → Use channel->asyncOpen2 in /dom/base/nsXMLHttpRequest.cpp
No longer depends on: 1190201
Bug 1182571 - XHR conversion to AsyncOpen2 Initial patch that sets the correct flags.
Bug 1182571 - Move channel initialization to Send(). r?ckerschb,sicking,bkelly AsyncOpen2 channels require all the flags to be set when the channel is created. Since XHR has a withCredentials attribute that script can change between open() and send(), switching to AsyncOpen2() requires moving channel creation from Open() to Send(). Unfortunately, since SetRequestHeader() is also called between Open() and Send() we can no longer directly set headers on the channel. This did end up simplifying the code though. XHR now uses the InternalHeaders class that was introduced for the Fetch API. This does appropriate header validation. I have introduced a Combine() method as per the Fetch specification. We set the headers on the channel in Send(). The necessity to support chrome XHR and XHR with expanded permissions, collectively identified by IsSystemXHR() required the introduction of a new Header guard |request-system|. System XHR can call SetRequestHeader() with invalid request headers and those headers will actually be set on the channel. The only restriction is that a forbidden header overwrites an earlier header instead of combining with it.
Bug 1182571 Refactor upload Move upload to separate function.
These set of patches work for the most part. The biggest missing piece is not setting up preflights correctly. The way this is done internally by asyncOpen2 is incorrect since it does not deal with the preflight case, using the second form of the nsCORSListenerProxy constructor that accepts the list of unsafe headers. The intention is that preflights will be fixed by Bug 1199049. Unassigning myself for now.
Assignee: nsm.nikhil → nobody
Comment on attachment 8650035 [details] [diff] [review] bug_1182571_asyncopen2_xmlhttprequest.patch Review of attachment 8650035 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsXMLHttpRequest.cpp @@ +1548,3 @@ > if (IsSystemXHR()) { > if (!nsContentUtils::IsSystemPrincipal(mPrincipal)) { > nsIScriptSecurityManager *secMan = nsContentUtils::GetSecurityManager(); I think we can yes. If we use the SEC_ALLOW_CROSS_ORIGIN flag when IsSystemXHR() is true. @@ +1563,2 @@ > if (nsContentUtils::CheckMayLoad(mPrincipal, aChannel, true)) { > return NS_OK; Yup @@ +1564,5 @@ > return NS_OK; > } > > // This is a cross-site request > mState |= XML_HTTP_REQUEST_USE_XSITE_AC; We will need to set this flag though. At least until the channel can take care of preventing redirects of preflighted channels. @@ +1747,5 @@ > // will be automatically aborted if the user leaves the page. > nsCOMPtr<nsILoadGroup> loadGroup = GetLoadGroup(); > > nsSecurityFlags secFlags = nsILoadInfo::SEC_NORMAL; > + // (mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT); We won't know here if we need to preflight or not unfortunately. We don't know that for certain until send() is called. @@ -1751,5 @@ > // principal. Hence we set the sandbox flag in loadinfo, so that > // GetChannelResultPrincipal will give us the nullprincipal. > secFlags |= nsILoadInfo::SEC_SANDBOXED; > - } else { > - secFlags |= nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL; We can't remove this flag. We have to set this to ensure that the parsed document will have the right principal.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Depends on: 1207556
Fix the API for getting contentPolicyType from loadinfo. Currently lots of code is calling GetContentPolicyType and expecting that to give the detailed contentpolicy type. Most notably code that forwards the contentpolicytype to a new loadinfo/channel is doing this. However GetContentPolicyType actually returns the *external* contentPolicyType. I.e. things like TYPE_INTERNAL_XMLHTTPREQUEST and TYPE_INTERNAL_EVENTSOURCE is collapsed into TYPE_XMLHTTPREQUEST. What this code should actually be calling is InternalContentPolicyType. I've fixed all callsites and also renamed GetContentPolicyType to GetExternalContentPolicyType to make sure this is more clear. I also fixed one place in nsNetUtils where we shouldn't be calling GetContentPolicyType at all, but rather forward the loadinfo.
Attached patch Use AsyncOpen2 (obsolete) — — Splinter Review
This turned out to be very similar to Nikhil's first patch. This one is more polished and also contains a bit more code simplification. I don't think Nikhil's other patches are needed to fix this bug, and I think they need significantly more work in order to work, so marking them obsolete.
Assignee: mozilla → jonas
Attachment #8650035 - Attachment is obsolete: true
Attachment #8659364 - Attachment is obsolete: true
Attachment #8659365 - Attachment is obsolete: true
Attachment #8659366 - Attachment is obsolete: true
Attachment #8668361 - Flags: review?(ehsan)
Comment on attachment 8668358 [details] [diff] [review] Fix nsILoadInfo->GetContentPolicyType API to be less ambigious Review of attachment 8668358 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/LoadInfo.cpp @@ +40,5 @@ > { > MOZ_ASSERT(mLoadingPrincipal); > MOZ_ASSERT(mTriggeringPrincipal); > > + MOZ_ASSERT(mContentPolicyType != 11); Oops, forgot to remove this. Please disregard.
Comment on attachment 8668361 [details] [diff] [review] Use AsyncOpen2 Review of attachment 8668361 [details] [diff] [review]: ----------------------------------------------------------------- This needs more work.
Attachment #8668361 - Flags: review?(ehsan)
Attached patch Patch to fix — — Splinter Review
Attachment #8668361 - Attachment is obsolete: true
Attachment #8669160 - Flags: review?(ehsan)
Comment on attachment 8668358 [details] [diff] [review] Fix nsILoadInfo->GetContentPolicyType API to be less ambigious Review of attachment 8668358 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Jonas, that makes things more clear. ::: netwerk/base/LoadInfo.cpp @@ +40,5 @@ > { > MOZ_ASSERT(mLoadingPrincipal); > MOZ_ASSERT(mTriggeringPrincipal); > > + MOZ_ASSERT(mContentPolicyType != 11); Sure, but don't forget to remove this before you land. @@ +248,2 @@ > { > *aResult = nsContentUtils::InternalContentPolicyTypeToExternal(mContentPolicyType); Since we are updating this to be less ambigious, could you please also renamve mContentPolicyType to mInternalContentPolicyType? ::: netwerk/base/nsNetUtil.inl @@ +230,5 @@ > + rv = channel->SetLoadFlags(aLoadFlags | (normalLoadFlags & nsIChannel::LOAD_REPLACE)); > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + channel.forget(outChannel); Now since we are having a loadinfo on at least all of gecko internal channels that is a legitimate change because the assertion > NS_ENSURE_ARG_POINTER(aLoadInfo); within nsIOService::NewChannelFromURIWithLoadInfo is not going to fire for debug builds. ::: toolkit/modules/addons/WebRequest.jsm @@ +248,5 @@ > let channel = request.QueryInterface(Ci.nsIHttpChannel); > let loadContext = this.getLoadContext(channel); > let browser = loadContext ? loadContext.topFrameElement : null; > let loadInfo = channel.loadInfo; > + let policyType = loadInfo.externalContentPolicyType; Your change is fine, but shouldn't we check that the loadInfo is non null here?
Attachment #8668358 - Flags: review?(mozilla) → review+
Comment on attachment 8669160 [details] [diff] [review] Patch to fix Review of attachment 8669160 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsXMLHttpRequest.cpp @@ +2782,5 @@ > + // SEC_REQUIRE_CORS_WITH_CREDENTIALS flag to when the channel is > + // created. So set it here using a hacky internal API. > + nsCOMPtr<nsILoadInfo> loadInfo; > + mChannel->GetLoadInfo(getter_AddRefs(loadInfo)); > + static_cast<LoadInfo*>(loadInfo.get())->SetWithCredentialsSecFlag(); Just a drive by question: why so complicated? nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo(); loadInfo->SetWithCredentialsFlags(); Also, we have to make sure the loadInfo is non null, right?
Comment on attachment 8669160 [details] [diff] [review] Patch to fix Review of attachment 8669160 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsXMLHttpRequest.cpp @@ +2782,5 @@ > + // SEC_REQUIRE_CORS_WITH_CREDENTIALS flag to when the channel is > + // created. So set it here using a hacky internal API. > + nsCOMPtr<nsILoadInfo> loadInfo; > + mChannel->GetLoadInfo(getter_AddRefs(loadInfo)); > + static_cast<LoadInfo*>(loadInfo.get())->SetWithCredentialsSecFlag(); What Christoph said, also please make sure to add the null check. ::: dom/security/nsContentSecurityManager.cpp @@ +169,5 @@ > > if (internalContentPolicyType == > + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST || > + internalContentPolicyType == > + nsIContentPolicy::TYPE_XMLHTTPREQUEST) { This second condition should never happen since you're dealing with the internal type, and if we get TYPE_XMLHTTPREQUEST here, a caller somewhere has been passing in the wrong content policy type. Please remove the condition. I may be a good idea to add a MOZ_ASSERT here to make sure we indeed never see TYPE_XMLHTTPREQUEST here. @@ +170,5 @@ > if (internalContentPolicyType == > + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST || > + internalContentPolicyType == > + nsIContentPolicy::TYPE_XMLHTTPREQUEST) { > + mimeTypeGuess = EmptyCString(); Hmm, why this change? (I guess it's more correct!) ::: netwerk/base/LoadInfo.h @@ +53,5 @@ > already_AddRefed<nsILoadInfo> Clone() const; > > + // This function is the *only* function which can change the securityflags > + // of a loadinfo. It only exists because of the XHR code. Don't call it > + // from anywhere else! Sigh. Let's make this a bit more bullet proof. Can you please make this function private and add nsXMLHttpRequest as a friend?
Attachment #8669160 - Flags: review?(ehsan) → review+
> > + // SEC_REQUIRE_CORS_WITH_CREDENTIALS flag to when the channel is > > + // created. So set it here using a hacky internal API. > > + nsCOMPtr<nsILoadInfo> loadInfo; > > + mChannel->GetLoadInfo(getter_AddRefs(loadInfo)); > > + static_cast<LoadInfo*>(loadInfo.get())->SetWithCredentialsSecFlag(); > > What Christoph said, also please make sure to add the null check. I don't think that should be needed. Since the XHR code calls NS_NewChannel, there should be no way that mChannel doesn't have a loadInfo. No? > ::: dom/security/nsContentSecurityManager.cpp > @@ +169,5 @@ > > > > if (internalContentPolicyType == > > + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST || > > + internalContentPolicyType == > > + nsIContentPolicy::TYPE_XMLHTTPREQUEST) { > > This second condition should never happen since you're dealing with the > internal type, and if we get TYPE_XMLHTTPREQUEST here, a caller somewhere > has been passing in the wrong content policy type. Please remove the > condition. My concern is addons. It seems quite likely that they would use nsIContentPolicy::TYPE_XMLHTTPREQUEST if they are creating network requests. Or do we want them to also use nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST? And assert if they don't? > @@ +170,5 @@ > > if (internalContentPolicyType == > > + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST || > > + internalContentPolicyType == > > + nsIContentPolicy::TYPE_XMLHTTPREQUEST) { > > + mimeTypeGuess = EmptyCString(); > > Hmm, why this change? (I guess it's more correct!) Because it matches what the XHR code currently uses, and addons might depend on it. The reason that we were previously using "application/xml" here was that that matches what XMLDocument.load() uses. So this *does* change the mimeguess that addons see from XMLDocument.load(). But that felt less important than making sure that XHR requests aren't changed from an addon perspective. > Let's make this a bit more bullet proof. Can you please make this function > private and add nsXMLHttpRequest as a friend? Will do.
(In reply to Jonas Sicking (:sicking) from comment #15) > > > + // SEC_REQUIRE_CORS_WITH_CREDENTIALS flag to when the channel is > > > + // created. So set it here using a hacky internal API. > > > + nsCOMPtr<nsILoadInfo> loadInfo; > > > + mChannel->GetLoadInfo(getter_AddRefs(loadInfo)); > > > + static_cast<LoadInfo*>(loadInfo.get())->SetWithCredentialsSecFlag(); > > > > What Christoph said, also please make sure to add the null check. > > I don't think that should be needed. Since the XHR code calls NS_NewChannel, > there should be no way that mChannel doesn't have a loadInfo. No? Yeah, good point. > > ::: dom/security/nsContentSecurityManager.cpp > > @@ +169,5 @@ > > > > > > if (internalContentPolicyType == > > > + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST || > > > + internalContentPolicyType == > > > + nsIContentPolicy::TYPE_XMLHTTPREQUEST) { > > > > This second condition should never happen since you're dealing with the > > internal type, and if we get TYPE_XMLHTTPREQUEST here, a caller somewhere > > has been passing in the wrong content policy type. Please remove the > > condition. > > My concern is addons. It seems quite likely that they would use > nsIContentPolicy::TYPE_XMLHTTPREQUEST if they are creating network requests. > Or do we want them to also use > nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST? And assert if they don't? That's fair, but why not just convert the internal type to external and then compare the return value against TYPE_XMLHTTPREQUEST? The reason why I don't like the code above is that it sets a precedence towards mixing the two content policy types. > > @@ +170,5 @@ > > > if (internalContentPolicyType == > > > + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST || > > > + internalContentPolicyType == > > > + nsIContentPolicy::TYPE_XMLHTTPREQUEST) { > > > + mimeTypeGuess = EmptyCString(); > > > > Hmm, why this change? (I guess it's more correct!) > > Because it matches what the XHR code currently uses, and addons might depend > on it. The reason that we were previously using "application/xml" here was > that that matches what XMLDocument.load() uses. > > So this *does* change the mimeguess that addons see from XMLDocument.load(). > But that felt less important than making sure that XHR requests aren't > changed from an addon perspective. That's fair.
> That's fair, but why not just convert the internal type to external and then > compare the return value against TYPE_XMLHTTPREQUEST? The reason why I > don't like the code above is that it sets a precedence towards mixing the > two content policy types. I'm not sure I understand what you propose. We want TYPE_XMLHTTPREQUEST and TYPE_INTERNAL_XMLHTTPREQUEST to use "" as mimetype-guess. But TYPE_INTERNAL_EVENTSOURCE to use "text/event-stream". Both TYPE_INTERNAL_XMLHTTPREQUEST and TYPE_INTERNAL_EVENTSOURCE map to TYPE_XMLHTTPREQUEST. I'm happy to do any comparisons which result in that behavior.
(In reply to Jonas Sicking (:sicking) from comment #18) > > That's fair, but why not just convert the internal type to external and then > > compare the return value against TYPE_XMLHTTPREQUEST? The reason why I > > don't like the code above is that it sets a precedence towards mixing the > > two content policy types. > > I'm not sure I understand what you propose. > > We want TYPE_XMLHTTPREQUEST and TYPE_INTERNAL_XMLHTTPREQUEST to use "" as > mimetype-guess. But TYPE_INTERNAL_EVENTSOURCE to use "text/event-stream". > > Both TYPE_INTERNAL_XMLHTTPREQUEST and TYPE_INTERNAL_EVENTSOURCE map to > TYPE_XMLHTTPREQUEST. Oh, sorry I forgot about that issue. Yeah now I can't see any other way to do this, so I guess the current code is fine. But maybe add a comment explaining why the comparison has to work this way?
(In reply to Jonas Sicking (:sicking) from comment #15) > I don't think that should be needed. Since the XHR code calls NS_NewChannel, > there should be no way that mChannel doesn't have a loadInfo. No? One last thought on that. An addon that implements it's own protocol handler and also implements NewChannel2() but doesn't attach a loadinfo within NewChannel2(). I agree it's a rare case, but possible. I would rather worry now than being sorry afterwards. And an additional nullcheck doesn't cost that much, does it?
That *should* crash. We want to eventually get rid of requests that don't have a loadinfo, so no point in ever supporting such protocol handlers.
Jonas, I just realized we have to pass the *internalContentPolicyType* to NS_CheckContentLoadPolicy() within the contentSecurityManager. Can you please update that part before you land?
Flags: needinfo?(jonas)
Attached patch Additional fixes — — Splinter Review
This is additional fixes, to be applied on top of the previous patch, in order to pass tests. There's a few changes here: * Only call loadInfo->SetWithCredentialsSecFlag() if we're actually doing a CORS request. I.e. not for system XHRs. * Fix a leak that was happening when xhr.send() bails early. The problem is that the XHR object holds on to the channel through mChannel, and the channel holds on to XHR through notificationCallbacks. This change is not really related to the use of AsyncOpen2. Since some checks were moved out of XHR::Send and into AsyncOpen2, we now ended up creating the cycle in situations where we previously didn't. * Make nsCORSListenerProxy do all of its redirect security checks before forwarding the AsyncOnChannelRedirect to the XHR object. Otherwise the XHR object thinks that the redirect will happen and updates mChannel to a channel that will never get used. This breaks XHR::OnStart/StopRequest. This change also happens to simplify nsCORSListenerProxy since we no longer need to run code once AsyncOnChannelRedirect succeeds. * Update a few tests that are now passing. When an XHR request is made cross-site and the XHR object has upload event listeners, the spec requires that a preflight is made, even if one would otherwise not have been needed. We used to check for upload listeners every time there was a redirect, which meant that we did more preflights than needed. Now just check for upload listeners when xhr.send() is called. If listeners are added later we simply don't fire them. This causes us to pass a few tests that we previously weren't. This especially affects requests from workers since the worker-xhr code adds internal upload listeners after xhr.send() is called. So cross-site XHR requests in workers were fairly busted if they got redirected. * The previous bullet actually also causes us to *fail* to web-platform tests. There are a couple of CSP-in-workers tests which we previously only passed because the XHR requests were blocked by the CORS code. We haven't yet implemented CSP in workers, so the only reason that the tests were passing was that our CORS implementation in workers were busted and blocked these requests. (These failures were a huge pain to figure out). * Fix a few tests which expected xhr.open to throw if there's a CSP policy in place which prevents the given URL from being opened. Since the nsIContentPolicy checks have been moved to AsyncOpen2 it is now instead xhr.send() which throws. I think the spec actually says that neither should throw and that we instead should treat it like a network error and fire an "error" event. But doing that would be for another bug.
Flags: needinfo?(jonas)
Attachment #8674092 - Flags: review?(ehsan)
That last comment is for a different bug
(In reply to Jonas Sicking (:sicking) from comment #26) > * The previous bullet actually also causes us to *fail* to web-platform > tests. > There are a couple of CSP-in-workers tests which we previously only passed > because the XHR requests were blocked by the CORS code. We haven't yet > implemented CSP in workers, so the only reason that the tests were passing > was that our CORS implementation in workers were busted and blocked these > requests. (These failures were a huge pain to figure out). Kate, you might be interested in this!
Comment on attachment 8674092 [details] [diff] [review] Additional fixes Review of attachment 8674092 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsXMLHttpRequest.cpp @@ +2784,5 @@ > // can be modified after, so we don't know what to set the > // SEC_REQUIRE_CORS_WITH_CREDENTIALS flag to when the channel is > // created. So set it here using a hacky internal API. > + > + // Not doing this for system XHR uses since those don't use CORS. Nit: please move this comment to the line before the if condition. @@ +2900,5 @@ > > if (NS_WARN_IF(NS_FAILED(rv))) { > + // Drop our ref to the channel to avoid cycles. Also drop channel's > + // ref to us to be extra safe. > + mChannel->SetNotificationCallbacks(mNotificationCallbacks); Do you mind creating a RAII class for performing this? It's OK if you want to do that in a follow-up.
Attachment #8674092 - Flags: review?(ehsan) → review+
> Do you mind creating a RAII class for performing this? It's OK if you want to do that in a follow-up. We only need to do this if AsyncOpen2 fails. Once AsyncOpen2 has returned a success code, necko will take care of breaking the cycle. So it doesn't seem like RAII is what we want here?
(In reply to Jonas Sicking (:sicking) from comment #31) > > Do you mind creating a RAII class for performing this? It's OK if you want to do that in a follow-up. > > We only need to do this if AsyncOpen2 fails. Once AsyncOpen2 has returned a > success code, necko will take care of breaking the cycle. So it doesn't seem > like RAII is what we want here? I was mainly talking about a class that would do |mChannel->SetNotificationCallbacks(mNotificationCallbacks);| in its dtor if a .EverythingWasOK() method hasn't been called by the time the object goes away, so that we would call its .EverythingWasOK() after AsyncOpen2 finished. But this is just one code path we're talking about (at least for now) so up to you, I'm fine either way.
All of the previous failures were actually due to the "nsILoadInfo->GetContentPolicyType API" patch. First off I had missed a couple of js callsites that still used the old API. Second, we actually have one callsite of NS_NewChannelInternal(loadinfo) which is passing a null loadinfo. It's in one of the xpcshell tests. It's probably a good idea to keep NS_NewChannelInternal(loadinfo) accepting null. That way channel code can call it when forwarding to inner channels, which was the case here. The question is if we should do that by changing NS_NewChannelInternal(loadinfo), or by changing nsIOService::NewChannelFromURIWithLoadInfo. I would actually prefer to change nsIOService::NewChannelFromURIWithLoadInfo, since that means that JS channel code that needs to do forwarding can use ioService.newChannelFromURIWithLoadInfo. It also means keeping the C++ and JS APIs more consistent. Christoph, let me know what you think. I'm happy to leave nsIOService::NewChannelFromURIWithLoadInfo and instead change NS_NewChannelInternal.
Flags: needinfo?(jonas)
Attachment #8675211 - Flags: review?(mozilla)
(In reply to Jonas Sicking (:sicking) from comment #37) > Second, we actually have one callsite of NS_NewChannelInternal(loadinfo) > which is passing a null loadinfo. It's in one of the xpcshell tests. > > It's probably a good idea to keep NS_NewChannelInternal(loadinfo) accepting > null. That way channel code can call it when forwarding to inner channels, > which was the case here. I don't really like that actually. We should never have that case where a channel does not have a loadinfo attached within Gecko. What testcase is it? Probably we can fix the testcase. Alternatively, if it's only one testcase we could let that test run only on a release build. Other changes within the patch look reasonable. If we really have to make one function accept a null loadinfo, then I agree with your patch and we should change nsIOService::NewChannelFromURIWithLoadInfo.
Flags: needinfo?(jonas)
This is the test: http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_packaged_app_service.js#369 However I don't want to change the test since this functionality needs to still work due to addons. Hence it is good that we have tests for it. The test seems to be specifically written to ensure that this keeps working. In fact, I would argue that we should have *more* tests that tests that channels without loadinfo's work well. Otherwise we will break addons more than we should. Which has already happened. > If we really have to make > one function accept a null loadinfo, then I agree with your patch and we > should change nsIOService::NewChannelFromURIWithLoadInfo. We still have lots of functions that enable creating channels with a null loadinfo. In fact, almost all NS_NewChannel* functions and ioService.newChannel* functions allow it by simply passing null for all loadinfo-related arguments. The only for in this bug is if we should keep nsIOService::NewChannelFromURIWithLoadInfo in sync with NS_NewChannelInternal(loadinfo) by removing the nullcheck in nsIOService::NewChannelFromURIWithLoadInfo, or if we should make NS_NewChannelInternal(loadinfo) call something other than nsIOService::NewChannelFromURIWithLoadInfo when it is passed a null loadinfo. Sounds like you want to keep them in sync? In which case the current patch is good?
Flags: needinfo?(jonas)
I'm also totally happy to start migrating addons to always create channels with loadinfos. But I think that's a topic of a separate bug.
Comment on attachment 8675211 [details] [diff] [review] Additional fixes to nsILoadInfo patch Review of attachment 8675211 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jonas Sicking (:sicking) from comment #39) > This is the test: > http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/ > test_packaged_app_service.js#369 > > However I don't want to change the test since this functionality needs to > still work due to addons. Hence it is good that we have tests for it. The > test seems to be specifically written to ensure that this keeps working. If you look closely, that test doesn't run in debug builds, only in release builds, so it should not be a problem to keep the assertion. > In fact, I would argue that we should have *more* tests that tests that > channels without loadinfo's work well. Otherwise we will break addons more > than we should. Which has already happened. I totally agree, in fact I already started working on such tests. > > If we really have to make > > one function accept a null loadinfo, then I agree with your patch and we > > should change nsIOService::NewChannelFromURIWithLoadInfo. > > We still have lots of functions that enable creating channels with a null > loadinfo. In fact, almost all NS_NewChannel* functions and > ioService.newChannel* functions allow it by simply passing null for all > loadinfo-related arguments. > > Sounds like you want to keep them in sync? In which case the current patch > is good? If you really want to take that assertion out, then I agree with the patch you have. Again, if it's only for the packaged_app_service.js test, then there is no need to take it out. Up to you.
Attachment #8675211 - Flags: review?(mozilla) → review+
NS_ENSURE_* are not assertions. They are runtime checks which return early in optimized builds as well.
(In reply to Jonas Sicking (:sicking) from comment #42) > NS_ENSURE_* are not assertions. They are runtime checks which return early > in optimized builds as well. Yeah, you are right, I though we have an assertion there. Then all good. Go ahead and land what you have here.
Depends on: 1216839
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: