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

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

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

[Rob Wu [:robwu]]

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>}

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1432205: Always require the creation of a LoadInfo in nsIOService::NewChannelFromURIWithProxyFlagsInternal. r=valentin

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc681c1b4807
Always require the creation of a LoadInfo in nsIOService::NewChannelFromURIWithProxyFlagsInternal. r=valentin,necko-reviewers

Comment 4

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/dc681c1b4807