Closed Bug 1432205 Opened 4 years ago Closed 1 year ago

Consider requiring the creation of a LoadInfo in nsIOService::NewChannelFromURIWithProxyFlagsInternal()

Categories

(Core :: Networking, enhancement, P3)

enhancement

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
Priority: -- → P3
Whiteboard: [necko-triaged]
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: 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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Blocks: 1716556
You need to log in before you can comment on or make changes to this bug.