1236900 - [Static Analysis][Dereference before null check] In function nsFrameLoader::GetNewTabContext, mOwnerContent is null checked

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable mOwnerContent is null checked:

>>if (mOwnerContent) {
>>    nsAutoString userContextIdStr;
>>    if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::usercontextid)) {
>>      mOwnerContent->GetAttr(kNameSpaceID_None,
>>                             nsGkAtoms::usercontextid,
>>                             userContextIdStr);

but earlier on is derefenced in function GetContainingApp that is called:
>>  nsCOMPtr<mozIApplication> containingApp = GetContainingApp();

In function GetContainingApp we have the dereference here:

>>  // See if our owner content's principal has an associated app.
>>  uint32_t appId = mOwnerContent->NodePrincipal()->GetAppId();

mOwnerContent is attributed a value in the constructor nsFrameLoader that is called from nsFrameLoader::Create:

>>  NS_ENSURE_TRUE(aOwner, nullptr);
>>  nsIDocument* doc = aOwner->OwnerDoc();
>>  NS_ENSURE_TRUE(!doc->IsResourceDoc() &&
>>                 ((!doc->IsLoadedAsData() && aOwner->GetComposedDoc()) ||
>>                   doc->IsStaticDocument()),
>>                 nullptr);
>>
>>  return new nsFrameLoader(aOwner, aNetworkCreated);

Each place whe  nsFrameLoader::Create gets called a valid aOwnver will be passed:
1.
>>      mFrameLoader = nsFrameLoader::Create(thisContent->AsElement(),
>>                                           mNetworkCreated);

2.
>>    nsFrameLoader* fl = nsFrameLoader::Create(content->AsElement(), false);
>>    if (fl) {

3.
>>  // frameloader exists.  It's more of a best-effort kind of thing.
>>  mFrameLoader = nsFrameLoader::Create(this, mNetworkCreated);
>>  if (mIsPrerendered) {

4.
>>    nsGenericHTMLFrameElement* dest =
>>      static_cast<nsGenericHTMLFrameElement*>(aDest);
>>    nsFrameLoader* fl = nsFrameLoader::Create(dest, false);

5.
>>        slots->mFrameLoader = nsFrameLoader::Create(this, false);
>>        NS_ENSURE_TRUE(slots->mFrameLoader, NS_OK);

Following the above scenarios the check from function GetNewTabContext only confuses the static analyzer. I advise eliminating it.

Comment 1

[Andi [:andi]]

Attachment: Bug 1236900.diff

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 8704125 [details] [diff] [review]
Bug 1236900.diff

I'm fine with this change given the call to GetContainingApp(), but I'm not convinced this code (before your change) is actually safe (though crash-stats tells me it's at least not far off). There's at least one case where we do set mOwnerContent to null (in StartDestroy()), but it's not at all clear how one would get a call in to GetNewTabContext() (or any of the other places where we dereference mOwnerContent w/o null checking it) after StartDestroy() has been called.

r=jst

Comment 3

[Andi [:andi]]

Attachment: Bug 1236900.diff

Thx for the review. I've inly updated the comment of the patch in order to include you as a reviewer.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d17f829b464

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d17f829b464