Closed
Bug 1432205
Opened 7 years ago
Closed 5 years ago
Consider requiring the creation of a LoadInfo in nsIOService::NewChannelFromURIWithProxyFlagsInternal()
Categories
(Core :: Networking, enhancement, P3)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla78
Tracking | Status | |
---|---|---|
firefox78 | --- | fixed |
People
(Reporter: bkelly, Assigned: ckerschb)
References
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file)
In bug 1231211 I am moving some code from nsIOService's NewChannelFromURIWithProxyFlags2() into a NewChannelFromProxyFlagsInternal(). While reviewing this Valentin noticed the comment is out of date here:
https://searchfox.org/mozilla-central/rev/e9a067a401e41017766f4ab90bc3906603273d51/netwerk/base/nsIOService.cpp#910-921
It seems like there is some code here trying to accomodate old addons and an API that no longer exists. We should consider trying to make this code require the creation of a LoadInfo
Comment 1•7 years ago
|
||
There is more incorrect comment/logic below the code referenced in comment 0.
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/nsIOService.cpp#835-842
// TYPE_DOCUMENT loads don't require a loadingNode or principal, but other
// types do.
if (aLoadingNode || aLoadingPrincipal ||
aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT) {
nsCOMPtr<nsINode> loadingNode(do_QueryInterface(aLoadingNode));
loadInfo = new LoadInfo(aLoadingPrincipal,
aTriggeringPrincipal,
loadingNode,
This claim about nullability of loading node and principal is also present in the LoadingNode documentation:
https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/netwerk/base/nsILoadInfo.idl#357-359
* For top-level loads, and for loads originating from workers, the
* LoadingNode is null. If the LoadingNode is non-null, then the
* LoadingPrincipal is the principal of the LoadingNode.
https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/netwerk/base/nsILoadInfo.idl#242-244
... For top-level loads, the
* LoadingPrincipal is null. For all loads except top-level loads
* the LoadingPrincipal is never null.
Despite the documentation, the implementation fails when the node and the principal are both null:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/LoadInfo.cpp#48-101
LoadInfo::LoadInfo(nsIPrincipal* aLoadingPrincipal,
nsIPrincipal* aTriggeringPrincipal,
nsINode* aLoadingContext,
...
: mLoadingPrincipal(aLoadingContext ?
aLoadingContext->NodePrincipal() : aLoadingPrincipal)
...
{
MOZ_ASSERT(mLoadingPrincipal);
MOZ_ASSERT(mTriggeringPrincipal);
#ifdef DEBUG
// TYPE_DOCUMENT loads initiated by javascript tests will go through
// nsIOService and use the wrong constructor. Don't enforce the
// !TYPE_DOCUMENT check in those cases
bool skipContentTypeCheck = false;
skipContentTypeCheck = Preferences::GetBool("network.loadinfo.skip_type_assertion");
#endif
(the DEBUG check was added in bug 1105556)
Apparently a different LoadInfo constructor should be used for TYPE_DOCUMENT:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/LoadInfo.cpp#279-283
The problem in this code path can be demonstrated as follows:
1. Enable devtools.chrome.enabled (e.g. via the checkbox at about:debugging).
2. Run the following from the global JS console:
Services.io.newChannelFromURIWithProxyFlags2(NetUtil.newURI("http://example.com/"), null, 0, null, null, null, 0, Components.interfaces.nsIContentPolicy.TYPE_DOCUMENT);
Expected result: a nsIChannel
Actual result: crash (nullptr dereference on release builds; I guess that on debug builds the assertion would be triggered).
Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
mozilla::net::LoadInfo::LoadInfo (this=0x7fffb7152300, aLoadingPrincipal=0x0, aTriggeringPrincipal=<optimized out>, aLoadingContext=0x0, aSecurityFlags=<optimized out>, aContentPolicyType=6, aLoadingClientInfo=..., aController=...)
at /path/to/gecko/netwerk/base/LoadInfo.cpp:247
247 mOriginAttributes = mLoadingPrincipal->OriginAttributesRef();
(gdb) p mLoadingPrincipal
$1 = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}
Assignee | ||
Comment 2•5 years ago
|
||
Updated•5 years ago
|
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/dc681c1b4807
Always require the creation of a LoadInfo in nsIOService::NewChannelFromURIWithProxyFlagsInternal. r=valentin,necko-reviewers
Comment 4•5 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox78:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in
before you can comment on or make changes to this bug.
Description
•