Closed Bug 1230351 Opened 4 years ago Closed 4 years ago

When visiting about:addons, my terminal fills with "WARNING: NS_ENSURE_TRUE(aSecondURI) failed: file dom/base/ThirdPartyUtil.cpp, line 49"


(Core :: DOM: Security, defect)

Not set



Tracking Status
firefox45 --- fixed


(Reporter: dholbert, Assigned: dholbert)




(2 files)

 1. Start a debug build with a fresh profile.
 2. Visit about:addons (e.g. via selecting Tools menu|Add-ons)
 3. Look at your terminal.

ACTUAL RESULTS: Many copies of this line are spammed to the terminal:
WARNING: NS_ENSURE_TRUE(aSecondURI) failed: file dom/base/ThirdPartyUtil.cpp, line 49

EXPECTED RESULTS: No such warning-spam

This NS_ENSURE_TRUE is in IsThirdPartyInternal() -- code-quote:
> 44 nsresult
> 45 ThirdPartyUtil::IsThirdPartyInternal(const nsCString& aFirstDomain,
> 46                                      nsIURI* aSecondURI,
> 47                                      bool* aResult)
> 48 {
> 49   NS_ENSURE_ARG(aSecondURI);

The passed in aSecondURI is indeed null here -- it's |parentURI| at this callsite:

> 146     parent = current->GetScriptableParent();
> 153     rv = GetURIFromWindow(parent, getter_AddRefs(parentURI));
> 154     NS_ENSURE_SUCCESS(rv, rv);
> 155 
> 156     rv = IsThirdPartyInternal(bottomDomain, parentURI, &result);

So, why is parentURI null? It's because the window it's associated with ("parent") has a nsSystemPrincipal as its principal, and nsSystemPrincipal::GetURI() returns null.  So, GetURIFromWindow succeeds and sets parentURI to null.

I'm unclear on why/whether this is a warn-worthy condition. Can we just quiet the NS_ENSURE_TRUE, and replace it with a null-check-and-return?

(hg blame says this NS_ENSURE_TRUE was added in bug 1033871 -- before that, it was a NS_ASSERTION. Hoping mcmanus, the reviewer on that bug, might have some idea of whether check-and-return-without-warning would be sane here.)
Here's the backtrace of the NS_ENSURE_TRUE failure, for the record. Note the "aSecondURI=0x0" at stack level 0 -- that's what causes the NS_ENSURE_TRUE to fail & spam a warning.
Comment on attachment 8695561 [details]
backtrace of NS_ENSURE_ARG failure

(sorry, all of my "NS_ENSURE_TRUE" mentions here really meant to say "NS_ENSURE_ARG")
Attachment #8695561 - Attachment description: backtrace of NS_ENSURE_TRUE failure → backtrace of NS_ENSURE_ARG failure
Here's a patch that preserves existing behavior, but removes the spammy warning.

(I could also see that we might want to return NS_OK & set *aResult=false in this scenario, too; not sure which would be better. So, I'm erring on the side of not changing behavior.)
Attachment #8695572 - Flags: review?(mcmanus)
Comment on attachment 8695572 [details] [diff] [review]
fix v1: just fail, without spamming a warning

the change is fine, but we should make sure blake expected this..
Attachment #8695572 - Flags: review?(mcmanus) → review?(mrbkap)
Attachment #8695572 - Flags: review?(mrbkap) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.