Nicholas Nethercote [inactive] Assignee
	Description • 8 years ago

nsIURI.getSpec is fallible, but plenty of call sites don't check the return value. Adding [must_use] to the IDL will identify those call sites.

See bug 1290350 comment 6 for an example of how this can cause problems.

	Nicholas Nethercote [inactive] Assignee
	Comment 1 • 8 years ago

Some of these are easy, i.e. when there's similar error checking in the same
function that I can copy.

Some of them can use GetSpecOr() (or whatever I end up calling it) because they
involve logging, and I missed them in bug 1297961's patch.

Some of them are harder and I'm not sure how to handle. These are marked with a
"njn" comment. Suggestions welcome.

	u408661
	Comment 2 • 8 years ago

Comment on attachment 8785171 [details] [diff] [review]
(part 1) - Add missing checks to GetSpec() calls in netwerk/

Review of attachment 8785171 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good to me. I want to get :mcmanus's opinion on some of your comments, as well, since he'll probably understand them better than me and may have a better idea of what to do.

Patrick - take a look at the "njn:" comments in the code and offer your opinion, if you could. Thanks!

::: netwerk/base/nsPACMan.cpp
@@ +452,5 @@
>        NS_NewURI(getter_AddRefs(pacURI), mPACURISpec);
>  
>        // NOTE: This results in GetProxyForURI being called
>        if (pacURI) {
> +        nsresult rv = pacURI->GetSpec(mNormalPACURISpec);

Add a MOZ_ASSERT(NS_SUCCEEDED(rv)) here, since there's no good reason GetSpec should fail

::: netwerk/base/nsPACMan.h
@@ +166,5 @@
>  
>      nsAutoCString tmp;
> +    // njn: not sure about this one. If it's really a PACURI and we return
> +    // false because GetSpec() fails, is that bad? I guess it depends on the
> +    // IsPACURI() call sites, and I don't know enough to evaluate them.

So I think this is fine the way you have it. The change to having GetSpec return NS_ERROR_OUT_OF_MEMORY already traded a potential crash here for a potential infinite loop (if I'm reading the one call site for this version of IsPACURI correctly), and this doesn't change that. Even if I'm wrong about an infinite loop, this won't significantly change the behavior in the one place this is called.

@@ +169,5 @@
> +    // false because GetSpec() fails, is that bad? I guess it depends on the
> +    // IsPACURI() call sites, and I don't know enough to evaluate them.
> +    nsresult rv = uri->GetSpec(tmp);
> +    if (NS_FAILED(rv))
> +      return false;

Braces around body, please. (I know it doesn't match style in the file, but we've been doing braces around body for all new things in necko for a while now.)

::: netwerk/base/nsURIHashKey.h
@@ +47,5 @@
>          nsAutoCString spec;
> +        // njn: not sure what to do here. It's conceivable that GetSpec() might
> +        // succeed for a particular URI at one point and then fail at another
> +        // (due to OOM), in which case a lookup might fail when it should
> +        // succeed.

It almost seems like we should be caching the PLDHashNumber here instead of regenerating it every time HashKey is called. That also seems kind of silly and like overkill. So I think we're probably OK here.

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +320,5 @@
>  
>    nsCString spec;
>    nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> +  // njn: not sure what to do here. Below we Cancel(rv), but without returning
> +  // early. Is that appropriate here?

Not as sure here. Let's ask :mcmanus who will probably know better than me here.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +2215,5 @@
>          // this is basically used for test coverage of an otherwise 'hintable' feature
> +        // njn: is using "[unknown URI spec]" on GetSpec() failure appropriate
> +        // here? I think so... the only "speculative-connect-request" observer
> +        // I can see is in netwerk/test/mochitests/test_rel_preconnect.html and
> +        // it doesn't look at the data argument.

I'm half-tempted to say just remove this use of GetSpec entirely, since it's not actually used. That gets a little more invasive, though (have to change NeckoParent & NeckoChild). If you don't want to bother with that, I'm fine leaving this one as you have it.

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp
@@ +120,5 @@
>      nsCOMPtr<nsIURI> uri = iter.Data();
>      SerializedURI serialized;
>      if (uri) {
> +      // njn: not sure what to do here. Change return type to nsresult and make
> +      // callers handle failure?

I like the idea of changing the return type, but I have no idea how to make the caller handle failure - it seems to be used for something to do with chrome urls

@@ +147,5 @@
>    mapping.scheme = mScheme;
>    mapping.path = aRoot;
>    if (aBaseURI) {
> +    // njn: not sure what to do here. Change return type to nsresult and make
> +    // callers handle failure?

Same as above.

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +240,5 @@
>  
>      nsAutoCString spec;
> +    rv = uri->GetSpec(spec);
> +    if (NS_FAILED(rv))
> +      return rv;

Braces around body, please

	Nicholas Nethercote [inactive] Assignee
	Comment 3 • 8 years ago

> ::: netwerk/base/nsPACMan.cpp
> @@ +452,5 @@
> >        NS_NewURI(getter_AddRefs(pacURI), mPACURISpec);
> >  
> >        // NOTE: This results in GetProxyForURI being called
> >        if (pacURI) {
> > +        nsresult rv = pacURI->GetSpec(mNormalPACURISpec);
> 
> Add a MOZ_ASSERT(NS_SUCCEEDED(rv)) here, since there's no good reason
> GetSpec should fail

I will revert my original change and make it a MOZ_RELEASE_ASSERT instead. That way we'll know via crash reports if it ever does fail.


> ::: netwerk/base/nsPACMan.h
> @@ +166,5 @@
> >  
> >      nsAutoCString tmp;
> > +    // njn: not sure about this one. If it's really a PACURI and we return
> > +    // false because GetSpec() fails, is that bad? I guess it depends on the
> > +    // IsPACURI() call sites, and I don't know enough to evaluate them.
> 
> So I think this is fine the way you have it. The change to having GetSpec
> return NS_ERROR_OUT_OF_MEMORY already traded a potential crash here for a
> potential infinite loop (if I'm reading the one call site for this version
> of IsPACURI correctly), and this doesn't change that. Even if I'm wrong
> about an infinite loop, this won't significantly change the behavior in the
> one place this is called.

I see two call sites for this function:

- One in nsPACMan::AsyncGetProxyForURI

- One in nsProtocolProxyService::Resolve_Internal


> ::: netwerk/protocol/http/nsHttpHandler.cpp
> @@ +2215,5 @@
> >          // this is basically used for test coverage of an otherwise 'hintable' feature
> > +        // njn: is using "[unknown URI spec]" on GetSpec() failure appropriate
> > +        // here? I think so... the only "speculative-connect-request" observer
> > +        // I can see is in netwerk/test/mochitests/test_rel_preconnect.html and
> > +        // it doesn't look at the data argument.
> 
> I'm half-tempted to say just remove this use of GetSpec entirely, since it's
> not actually used. That gets a little more invasive, though (have to change
> NeckoParent & NeckoChild). If you don't want to bother with that, I'm fine
> leaving this one as you have it.

I will remove it. Might as well fix it properly.


> ::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp
> @@ +120,5 @@
> >      nsCOMPtr<nsIURI> uri = iter.Data();
> >      SerializedURI serialized;
> >      if (uri) {
> > +      // njn: not sure what to do here. Change return type to nsresult and make
> > +      // callers handle failure?
> 
> I like the idea of changing the return type, but I have no idea how to make
> the caller handle failure - it seems to be used for something to do with
> chrome urls
> 
> @@ +147,5 @@
> >    mapping.scheme = mScheme;
> >    mapping.path = aRoot;
> >    if (aBaseURI) {
> > +    // njn: not sure what to do here. Change return type to nsresult and make
> > +    // callers handle failure?
> 
> Same as above.

In both cases I will change the return type from void to nsresult. It works out fine at the call sites.

	Nicholas Nethercote [inactive] Assignee
	Comment 4 • 8 years ago

New version with the aforementioned changes.

Bugzilla informs me that mcmanus is on PTO until September 6. hurley, could
anybody else help with the "njn" comments in the meantime?

	Nicholas Nethercote [inactive] Assignee
	Comment 6 • 8 years ago

This required making GetScriptLocation() fallible.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 7 • 8 years ago

Comment on attachment 8785824 [details] [diff] [review]
Add missing checks to GetSpec() calls in caps/ and js/

Review of attachment 8785824 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1693,5 @@
>          *anonymizeID += 1;
>      } else if (JSPrincipals* principals = JS_GetCompartmentPrincipals(c)) {
> +        nsresult rv = nsJSPrincipals::get(principals)->GetScriptLocation(name);
> +        if (NS_FAILED(rv)) {
> +            name.Assign("(unknown)");

AssignLiteral(...)

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 8 years ago

Comment on attachment 8786242 [details] [diff] [review]
Add missing checks to GetSpec() calls in embedding/

>+++ b/embedding/components/webbrowserpersist/WebBrowserPersistLocalDocument.cpp
>             newURI->GetSpec(uriSpec);
>+            NS_ENSURE_SUCCESS(rv, rv);

Need to actually assign to rv here.

r=me with that

	Nicholas Nethercote [inactive] Assignee
	Comment 11 • 8 years ago

> >+++ b/embedding/components/webbrowserpersist/WebBrowserPersistLocalDocument.cpp
> >             newURI->GetSpec(uriSpec);
> >+            NS_ENSURE_SUCCESS(rv, rv);
> 
> Need to actually assign to rv here.

Good catch! It should have been caught by the compiler... which made me realize that the patch in my queue that actually adds the [must_use] to nsIFile.spec got lost along the way :)

	Nicholas Nethercote [inactive] Assignee
	Comment 12 • 8 years ago

This is not a good patch -- a combination of guesses and "no ideas" on how to
handle GetSpec() failure. Suggestions are welcome!

	Nicholas Nethercote [inactive] Assignee
	Comment 13 • 8 years ago

I wasn't sure about all of these. Please note the "njn" comments.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 14 • 8 years ago

Comment on attachment 8786567 [details] [diff] [review]
Add missing checks to GetSpec() calls in parser/

Review of attachment 8786567 [details] [diff] [review]:
-----------------------------------------------------------------

feedback+ & r+ assuming the comments are addressed.

::: parser/html/nsHtml5StreamParser.cpp
@@ +286,5 @@
>        // UTF-8 for an ellipsis.
>        mViewSourceTitle.AssignLiteral("data:\xE2\x80\xA6");
>      } else {
> +      // njn: how to handle failure here?
> +      Unused << temp->GetSpec(mViewSourceTitle);

I suggest assigning "\xE2\x80\xA6" (ellipsis) to mViewSourceTitle on failure.

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +897,5 @@
>  nsHtml5TreeOpExecutor::ShouldPreloadURI(nsIURI *aURI)
>  {
>    nsAutoCString spec;
> +  // njn: how to handle failure here? would returning false on failure be ok?
> +  Unused << aURI->GetSpec(spec);

Returning false seems appropriate.

::: parser/html/nsHtml5TreeOperation.cpp
@@ +914,5 @@
>  
>        nsAutoCString spec;
> +      rv = uri->GetSpec(spec);
> +      // njn: return NS_OK here or rv? this function uses a mixture
> +      NS_ENSURE_SUCCESS(rv, NS_OK);

It shouldn't make a practical difference.
NS_ENSURE_SUCCESS(rv, rv);
seems more idiomatic.

::: parser/html/nsParserUtils.cpp
@@ +203,5 @@
>      // Now, set the base URI on all subtree roots.
>      if (aBaseURI) {
> +      // njn: this is just a guess on how to best handle failure here.
> +      nsresult rv2 = aBaseURI->GetSpec(spec);
> +      if (NS_SUCCEEDED(rv2)) {

I'm inclined to suggest
NS_ENSURE_SUCCESS(rv2, rv2);
in case it's unsafe to continue after an error.

	Daniel Holbert [:dholbert]
	Comment 15 • 8 years ago

Comment on attachment 8786681 [details] [diff] [review]
Add missing checks to GetSpec() calls in layout/

Review of attachment 8786681 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ErrorReporter.cpp
@@ +37,5 @@
>        nsAutoCString cSpec;
> +      // njn: not sure what to do here. Change return type to nsresult and then
> +      // abort early in ErrorReporter::OutputError()? If so, what should mURI
> +      // be set to in that case?
> +      Unused << mURI->GetSpec(cSpec);

I think we should react to GetSpec failure by purging this micro-cache (doing the equivalent of what its Run() method does, perhaps via refactoring Run() out to a new helper-function to share functionality) and then returning the empty string.

Alternately, we could use your new infallible GetSpecOrDefault() function here (from bug 1297961) -- BUT, I think that'd mean we'd regress perf a bit due to losing the nice & fast stack-allocated buffer in |nsAutoCString cSpec|, for the usual case where we're stringifying a short URL here & not running out of memory.

So, I think we should probably purge the cache here.  If you'd like to punt this to a followup bug (and mention the bug number in the comment here), that's fine with me, since your current patch isn't changing behavior at all.

::: layout/style/ServoStyleSheet.cpp
@@ +83,5 @@
>      new ThreadSafePrincipalHolder(aSheetPrincipal);
>  
>    nsCString baseString;
> +  // njn: how to handle failure here?
> +  Unused << aBaseURI->GetSpec(baseString);

To answer what's appropriate for a failure here, I'd need to know more about the (rust-backed?) implementation of the "Servo_StyleSheet_FromUTF8Bytes" function that we call a few lines down (where we use |baseString|). Right now, I have no idea how it uses this |baseString| variable -- or why it even needs it, given that it accepts |base| as well, which is the URI version of this string.  Seems like it might be able to pull out the string more lazily if/when it needs it, perhaps?

All of which is to say, I don't know how best to handle failure here.

So: please file a followup bug with needinfo=bholley or heycam, and mention that bug number in your comment here.  With that, I'm fine with this change (since your patch isn't actually modifying behavior or making this any worse than it already is -- it's just formalizing a sweep-the-error-under-the-rug that's already present).

::: layout/style/nsCSSRules.cpp
@@ +975,5 @@
>    nsIDocument *doc = aPresContext->Document();
>    nsIURI *docURI = doc->GetDocumentURI();
>    nsAutoCString docURISpec;
> +  if (docURI) {
> +    // njn: is returning false on failure appropriate here?

Yeah, this seems fine. This just determines whether a our document should have particular -moz-document() CSS rules applied (i.e. if its URL matches the URL for the document-specific CSS).

If we fail to apply those CSS rules because we can't produce our stringified URI, *shrug*, oh well.

Please remove your "is returning false appropriate?" comment, and replace it with something like:
  // If GetSpec fails (due to OOM), we just skip these URI-specific CSS rules.

::: layout/style/nsStyleStruct.cpp
@@ +185,5 @@
>      mURL->GetRef(cref);
>      cref.Insert('#', 0);
>    } else {
> +    // njn: not sure how to handle failure here
> +    Unused << mURL->GetSpec(cref);

This seems fine (and the "not sure how to handle failure" comment seems perhaps appropriate to leave behind), as long as this reliably leaves |cref| empty. (Does it?)

There only seem to be 2 callers:
 - one in animation code (where an empty string would just mean we fail to apply an animated filter/clip-path/etc)
 - one in nsComputedDOMStyle.cpp, which is actually guaranteed not to hit this codepath because it's guarded by an IsLocalRef() check.

Given that, it seems fine to fail by leaving |cref| empty (which we already do, really).

	u408661
	Comment 16 • 8 years ago

Comment on attachment 8785785 [details] [diff] [review]
(part 1) - Add missing checks to GetSpec() calls in netwerk/

Review of attachment 8785785 [details] [diff] [review]:
-----------------------------------------------------------------

OK, r=me once we've got opinions on the two things below. Valentin, Dragana, there are some changes going in on code you two seem to be the most recent/active people on, can you take a look at the 2 changes and give your opinions? (1 for Valentin, 1 for Dragana).

::: netwerk/base/nsPACMan.h
@@ +167,5 @@
>  
>      nsAutoCString tmp;
> +    // njn: not sure about this one. If it's really a PACURI and we return
> +    // false because GetSpec() fails, is that bad? I guess it depends on the
> +    // IsPACURI() call sites, and I don't know enough to evaluate them.

Looking at both call sites, I think returning false here is the right thing to do, so go ahead and remove this comment.

::: netwerk/base/nsURIHashKey.h
@@ +47,5 @@
>          nsAutoCString spec;
> +        // njn: not sure what to do here. It's conceivable that GetSpec() might
> +        // succeed for a particular URI at one point and then fail at another
> +        // (due to OOM), in which case a lookup might fail when it should
> +        // succeed.

I think this is probably OK (if we're OOM, we have larger problems than a changed hash), but let's ask Valentin (who knows most about our URI code these days).

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +320,5 @@
>  
>    nsCString spec;
>    nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> +  // njn: not sure what to do here. Below we Cancel(rv), but without returning
> +  // early. Is that appropriate here?

Dragana seems to have spent some time in FTP-land, let's ask her!

	Valentin Gosu [:valentin] (he/him)
	Comment 17 • 8 years ago

Comment on attachment 8785785 [details] [diff] [review]
(part 1) - Add missing checks to GetSpec() calls in netwerk/

Review of attachment 8785785 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #16)
> Comment on attachment 8785785 [details] [diff] [review]
> ::: netwerk/base/nsURIHashKey.h
> @@ +47,5 @@
> >          nsAutoCString spec;
> > +        // njn: not sure what to do here. It's conceivable that GetSpec() might
> > +        // succeed for a particular URI at one point and then fail at another
> > +        // (due to OOM), in which case a lookup might fail when it should
> > +        // succeed.
> 
> I think this is probably OK (if we're OOM, we have larger problems than a
> changed hash), but let's ask Valentin (who knows most about our URI code
> these days).

I agree. I don't see another way of properly handling this. I'd leave in a comment mentioning that this may be a cause for intermittent failures in OOM situations.
In place hashing could be a way to fix this.

	Nicholas Nethercote [inactive] Assignee
	Comment 18 • 8 years ago

> ::: parser/html/nsHtml5StreamParser.cpp
> @@ +286,5 @@
> >        // UTF-8 for an ellipsis.
> >        mViewSourceTitle.AssignLiteral("data:\xE2\x80\xA6");
> >      } else {
> > +      // njn: how to handle failure here?
> > +      Unused << temp->GetSpec(mViewSourceTitle);
> 
> I suggest assigning "\xE2\x80\xA6" (ellipsis) to mViewSourceTitle on failure.

Is that without the preceding "data:"? i.e. this?

>       mViewSourceTitle.AssignLiteral("\xE2\x80\xA6");

	Nicholas Nethercote [inactive] Assignee
	Comment 19 • 8 years ago

> ::: layout/style/ErrorReporter.cpp
> @@ +37,5 @@
> >        nsAutoCString cSpec;
> > +      // njn: not sure what to do here. Change return type to nsresult and then
> > +      // abort early in ErrorReporter::OutputError()? If so, what should mURI
> > +      // be set to in that case?
> > +      Unused << mURI->GetSpec(cSpec);
> 
> I think we should react to GetSpec failure by purging this micro-cache
> (doing the equivalent of what its Run() method does, perhaps via refactoring
> Run() out to a new helper-function to share functionality) and then
> returning the empty string.
> 
> Alternately, we could use your new infallible GetSpecOrDefault() function
> here (from bug 1297961) -- BUT, I think that'd mean we'd regress perf a bit
> due to losing the nice & fast stack-allocated buffer in |nsAutoCString
> cSpec|, for the usual case where we're stringifying a short URL here & not
> running out of memory.

How about this?

> nsAutoCString cSpec
> nsresult rv = mURI->GetSpec(cSpec);
> if (NS_FAILED(rv)) {
>   cSpec.AssignLiteral("[nsIURI::GetSpec failed]");
> }

That's like using GetSpecOrDefault(), but keeps the nsAutoCString usage.

	Nicholas Nethercote [inactive] Assignee
	Comment 20 • 8 years ago

> ::: layout/style/ServoStyleSheet.cpp
> @@ +83,5 @@
> >      new ThreadSafePrincipalHolder(aSheetPrincipal);
> >  
> >    nsCString baseString;
> > +  // njn: how to handle failure here?
> > +  Unused << aBaseURI->GetSpec(baseString);
> 
> To answer what's appropriate for a failure here, I'd need to know more about
> the (rust-backed?) implementation of the "Servo_StyleSheet_FromUTF8Bytes"
> function that we call a few lines down (where we use |baseString|). Right
> now, I have no idea how it uses this |baseString| variable -- or why it even
> needs it, given that it accepts |base| as well, which is the URI version of
> this string.  Seems like it might be able to pull out the string more lazily
> if/when it needs it, perhaps?
> 
> All of which is to say, I don't know how best to handle failure here.

Looking again, bubbling the failure out to the caller looks easy. I'll try that and post a new patch.

	Nicholas Nethercote [inactive] Assignee
	Comment 21 • 8 years ago

heycam, please see comment 20 for context.

I will fold this into the previous patch before landing.

	Daniel Holbert [:dholbert]
	Comment 22 • 8 years ago

(In reply to Nicholas Nethercote [:njn] from comment #19)
> How about this?
[...]
> > if (NS_FAILED(rv)) {
> >   cSpec.AssignLiteral("[nsIURI::GetSpec failed]");

That solution is fine with me. Thanks!

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 23 • 8 years ago

(In reply to Nicholas Nethercote [:njn] from comment #18)
> > ::: parser/html/nsHtml5StreamParser.cpp
> > @@ +286,5 @@
> > >        // UTF-8 for an ellipsis.
> > >        mViewSourceTitle.AssignLiteral("data:\xE2\x80\xA6");
> > >      } else {
> > > +      // njn: how to handle failure here?
> > > +      Unused << temp->GetSpec(mViewSourceTitle);
> > 
> > I suggest assigning "\xE2\x80\xA6" (ellipsis) to mViewSourceTitle on failure.
> 
> Is that without the preceding "data:"? i.e. this?
> 
> >       mViewSourceTitle.AssignLiteral("\xE2\x80\xA6");

Without "data:", i.e. mViewSourceTitle.AssignLiteral("\xE2\x80\xA6"), since upon failure here, we have something other than a data: URL.

	Nicholas Nethercote [inactive] Assignee
	Comment 24 • 8 years ago

Previous patch was incomplete. This one is better.

	Dragana Damjanovic [:dragana]
	Comment 26 • 8 years ago

Comment on attachment 8785785 [details] [diff] [review]
(part 1) - Add missing checks to GetSpec() calls in netwerk/

Review of attachment 8785785 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +320,5 @@
>  
>    nsCString spec;
>    nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> +  // njn: not sure what to do here. Below we Cancel(rv), but without returning
> +  // early. Is that appropriate here?

This is fine. We are just cleaning up the listener if we are diverting to parent.

	Timothy Nikkel (:tnikkel)
	Comment 27 • 8 years ago

Comment on attachment 8785795 [details] [diff] [review]
Add missing checks to GetSpec() calls in image/

So this patch means that we can have ImageCacheKey objects and imgRequest objects that are not fully initialized. If we can't compute the right hash for a ImageCacheKey we effectively can't do anything useful with that image, and who knows what other weird behaviour this would enable. Auditing the code for this would be a nightmare. Can't we just crash in this situation instead of trying to handle this failure? Because I don't think there is any way to reasonably handle failures in this situations.

	Nicholas Nethercote [inactive] Assignee
	Comment 30 • 8 years ago

> So this patch means that we can have ImageCacheKey objects and imgRequest
> objects that are not fully initialized. If we can't compute the right hash
> for a ImageCacheKey we effectively can't do anything useful with that image,
> and who knows what other weird behaviour this would enable. Auditing the
> code for this would be a nightmare.

Why is it a nightmare? You have to check the nsresult outparam of the ImageCacheKey and ImageURL constructors, but that's not so hard, and is easy to audit. And imgRequest::Init() already returns an nsresult. (This reminds me, I should make that return value MOZ_MUST_USE too.)

> Can't we just crash in this situation
> instead of trying to handle this failure? Because I don't think there is any
> way to reasonably handle failures in this situations.

That would be a shame because a decent chunk of these GetSpec() OOMs are being hit in this case.

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 8 years ago

Comment on attachment 8788333 [details] [diff] [review]
Add missing checks to more GetSpec() calls in dom/base/

>@@ -3587,17 +3587,18 @@ nsContentUtils::ReportToConsoleNonLocali

I think you should use GetSpecOrDefault() here.  No reason to suppress the console report completely just because the associated URI is too long and we're too fragmented.

> nsContentUtils::GetWrapperSafeScriptFilename(nsIDocument *aDocument,
>+  // njn: how to handle this?

Arguably by not executing the relevant script.  Or by removing this function altogether; it's not clear to me whether it does anything useful nowadays...  Followup bugs are probably fine.

>@@ -9176,19 +9179,17 @@ nsContentUtils::IsSpecificAboutPage(JSOb

This function is buggy.  It will fail for things like "about:checkerboard#foo", for example.

It should probably use GetSpecIgnoringRef instead of GetSpec.  Followup here too?

> nsIDocument::GetDocumentURI(nsString& aDocumentURI) const
>+    // njn: how to handle failure here?

Make this funcation fallible.  I just looked through the callers and some of them want to throw exceptions to JS if this fails anyway, some shouldn't be using this function at all because it causes extra encoding conversions (NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(DOMEventTargetHelper), restyle logging, style context asserts (via GetURL), global window code checking for "about:neterror" (via GetURL) I'm looking at you), some could start throwing exceptions to JS even though they don't right now.  

Again, followup is ok.

> nsIDocument::GetDocumentURIFromJS(nsString& aDocumentURI) const
>+  // njn: how to handle failure here?

Mark documentURI as [Throws] in Document.webidl and throw on failure.  This will also apply to the branch in this unction that calls nsIDocument::GetDocumentURI, per above.  ;)

>@@ -10142,17 +10144,21 @@ nsDocument::SetScrollToRef(nsIURI *aDocu
>+  // njn: is this appropriate? The only caller of SetScrollToRef() is

If GetSpec fails, I'd just GetRef(mScrollToRef) and return (and ignore the return value from GetRef).

In a sane world, this function would _just_ GetRef(mScrollToRef)...  :(

> nsINode::GetBaseURI(nsAString &aURI) const
>+    // njn: how to handle failure?

Make the function fallible, maybe?  Followup is good here; I don't know what some of the callers care about.

> nsINode::GetBaseURIFromJS(nsAString& aURI) const
>+    // njn: how to handle failure?

Mark the Node.baseURI attribute [Throws] in Node.webidl and propagate the failure out.

> GetExtensionFromURI(nsIURI* uri, nsCString& ext)
>+    // njn: how to handle failure?

Return empty string in "ext".  It'll mean we sometimes incorrectly think a plugin is not enabled for the URI when it should be, but I can live with that.

>@@ -1545,17 +1549,18 @@ nsObjectLoadingContent::MaybeRewriteYout
>+  // njn: how to handle failure?

Just return without rewriting anything.

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 8 years ago

Comment on attachment 8788339 [details] [diff] [review]
Add missing checks to more GetSpec() calls in dom/xbl/

>@@ -518,17 +518,18 @@ NS_NewXBLProtoImpl(nsXBLPrototypeBinding
>+    NS_ENSURE_SUCCESS(rv, rv);

If only it were so simple.  The callers of this method ignore the nsresult return value _and_ one of them assumes that the aBinding->SetImplementation() call happens and will crash with null deref if not, while the other assumes that *aResult is set and will crash with null deref if not.

So this either needs more invasive changes or an explicit MOZ_CRASH or something.

>@@ -386,17 +386,20 @@ nsXBLProtoImplField::InstallField(JS::Ha

The URI is only used for the "filename" bit for the JS compilation.  It might be ok to use the default thing here instead...

Then again, just not installing the field is probably ok.

r- for that NS_NewXBLProtoImpl bit....

	Timothy Nikkel (:tnikkel)
	Comment 33 • 8 years ago

(In reply to Nicholas Nethercote [:njn] from comment #30)
> > So this patch means that we can have ImageCacheKey objects and imgRequest
> > objects that are not fully initialized. If we can't compute the right hash
> > for a ImageCacheKey we effectively can't do anything useful with that image,
> > and who knows what other weird behaviour this would enable. Auditing the
> > code for this would be a nightmare.
> 
> Why is it a nightmare? You have to check the nsresult outparam of the
> ImageCacheKey and ImageURL constructors, but that's not so hard, and is easy
> to audit. And imgRequest::Init() already returns an nsresult. (This reminds
> me, I should make that return value MOZ_MUST_USE too.)

Okay, then we'll need to make imgRequest::Init MOZ_MUST_USE (none of the callers currently check the return value) as well as any function you added a new return to. Because otherwise we are just moving the behaviour somewhere else.

	Nicholas Nethercote [inactive] Assignee
	Comment 34 • 8 years ago

Dragana, can you please review the FTPChannelChild.cpp change?

	Nicholas Nethercote [inactive] Assignee
	Comment 35 • 8 years ago

bz, thank you for the detailed comments. This bug is a slog and there are still numerous places in dom/ that I haven't looked at yet :(


> >@@ -3587,17 +3587,18 @@ nsContentUtils::ReportToConsoleNonLocali
> 
> I think you should use GetSpecOrDefault() here.  No reason to suppress the
> console report completely just because the associated URI is too long and
> we're too fragmented.

In other patches in this bug where the GetSpec() result was passed to InitWithWindowID() I did an early return like this, because I wasn't sure if the spec was just used in error messages, or something else. If you're confident that it's just for error messages, I will change this one and the others likewise.


> > nsContentUtils::GetWrapperSafeScriptFilename(nsIDocument *aDocument,
> >+  // njn: how to handle this?
> 
> Arguably by not executing the relevant script.  Or by removing this function
> altogether; it's not clear to me whether it does anything useful nowadays...
> Followup bugs are probably fine.

I filed bug 1301251.


> >@@ -9176,19 +9179,17 @@ nsContentUtils::IsSpecificAboutPage(JSOb
> 
> This function is buggy.  It will fail for things like
> "about:checkerboard#foo", for example.
> 
> It should probably use GetSpecIgnoringRef instead of GetSpec.  Followup here
> too?

I just changed it to use GetSpecIgnoringRef(). Might as well get do it right now.

 
> > nsIDocument::GetDocumentURI(nsString& aDocumentURI) const
> >+    // njn: how to handle failure here?
> 
> Make this funcation fallible.  I just looked through the callers and some of
> them want to throw exceptions to JS if this fails anyway, some shouldn't be
> using this function at all because it causes extra encoding conversions
> (NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(DOMEventTargetHelper),
> restyle logging, style context asserts (via GetURL), global window code
> checking for "about:neterror" (via GetURL) I'm looking at you), some could
> start throwing exceptions to JS even though they don't right now.  
> 
> Again, followup is ok.

I filed bug 1301249.


> > nsINode::GetBaseURI(nsAString &aURI) const
> >+    // njn: how to handle failure?
> 
> Make the function fallible, maybe?  Followup is good here; I don't know what
> some of the callers care about.

I filed bug 1301254.


I followed all your other suggestions.

	Nicholas Nethercote [inactive] Assignee
	Comment 36 • 8 years ago

bz, there were enough changes that I'd like another review. As comment 35 says,
I've made all the changes you suggested except for the InitWithWindowID() one,
pending your response to my question about that.

	Nicholas Nethercote [inactive] Assignee
	Comment 37 • 8 years ago

I made NS_NewXBLProtoImpl() just crash if its GetSpec() call fails. I also
changed its return type to |void|, because that's what it effectively is
anyway, so might as well make that clear.

	Cameron McCormack (:heycam)
	Comment 38 • 8 years ago

Comment on attachment 8788323 [details] [diff] [review]
Add missing checks to more GetSpec() calls in layout/

Review of attachment 8788323 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsLayoutStylesheetCache.cpp
@@ +949,5 @@
>      sheet->AsGecko()->ReparseSheet(sheetText);
>    } else {
> +    nsresult rv = sheet->AsServo()->ParseSheet(sheetText, uri, uri, nullptr, 0);
> +    // XXX: unclear how to handle failure better than by aborting!
> +    MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));

The URIs we use for the preference style sheets is about:PreferenceStyleSheet, so we'll use an nsSimpleURI, which can fail only on OOM.  If we are OOM this early, we've got problems, so release asserting is fine.

So replace the XXX with a comment like:

  // Parsing the about:PreferenceStyleSheet URI can only fail on OOM.
  // If we are OOM before we even parsed any documents, we may as well
  // abort.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 8 years ago

> If you're confident that it's just for error messages

For this case, I'm very confident, yes.  This is sending off a message to the console.

	Nicholas Nethercote [inactive] Assignee
	Comment 40 • 8 years ago

tnikkel, here's a version with more MOZ_MUST_USE additions and some extra
checking.

	Timothy Nikkel (:tnikkel)
	Comment 41 • 8 years ago

Comment on attachment 8789192 [details] [diff] [review]
Add missing checks to GetSpec() calls in image/

> nsMozIconURI::Equals(nsIURI* other, bool* result)
> {
>   NS_ENSURE_ARG_POINTER(other);
>   NS_PRECONDITION(result, "null pointer");
> 
>   nsAutoCString spec1;
>   nsAutoCString spec2;
> 
>-  other->GetSpec(spec2);
>-  GetSpec(spec1);
>+  nsresult rv = GetSpec(spec1);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = other->GetSpec(spec2);
>+  NS_ENSURE_SUCCESS(rv, rv);

Some places use nsIURI::Equals without checking the return value, so we'd have to set the result to false for them if one of these NS_ENSURE_SUCCESS fails.

	Nicholas Nethercote [inactive] Assignee
	Comment 42 • 8 years ago

> Some places use nsIURI::Equals without checking the return value, so we'd
> have to set the result to false for them if one of these NS_ENSURE_SUCCESS
> fails.

Good catch. I've added that locally to my patch.

	Dragana Damjanovic [:dragana]
	Comment 43 • 8 years ago

Comment on attachment 8788699 [details] [diff] [review]
Add missing checks to GetSpec() calls in netwerk/

Review of attachment 8788699 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on FTPChannelChild.cpp only. I have not look into other parts.

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 8 years ago

Comment on attachment 8789173 [details] [diff] [review]
Add missing checks to more GetSpec() calls in dom/base/

>+  [Pure, Throws]
>   readonly attribute DOMString URL;

I'm not sure how that compiles, since you did not change the GetURL signature in nsIDocument.h....

r=me with that fixed and the initwithwindowid bits fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 45 • 8 years ago

Comment on attachment 8789185 [details] [diff] [review]
Add missing checks to more GetSpec() calls in dom/xbl/

>+    NS_RELEASE_ASSERT(NS_SUCCEEDED(rv));

It's MOZ_RELEASE_ASSERT.

r=me with that fixed.

	Pulsebot
	Comment 46 • 8 years ago

Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe1cc85a7e6
Add missing checks to GetSpec() calls in netwerk/. r=hurley,dragana.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee83d585ff9d
Add missing checks to GetSpec() calls in caps/ and js/. r=mrbkap.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6663520be22
Add missing checks to GetSpec() calls in rdf/, startupcache/ and xpfe/. r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/253cccc6907d
Add missing checks to GetSpec() calls in embedding/. r=bz.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee484c1a00d
Add missing checks to GetSpec() calls in parser/. r=hsivonen.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f97c038c911d
Add missing checks to GetSpec() calls in layout/. r=dholbert,heycam.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e729283e0726
Add missing checks to GetSpec() calls in dom/base/. r=bz.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e7ef6f7289
Add missing checks to GetSpec() calls in dom/xbl/. r=bz.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8fd0a41c62f
Add missing checks to GetSpec() calls in toolkit/. r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d008348cd4f4
Add missing checks to GetSpec() calls in image/. r=tnikkel.

	Carsten Book [:Tomcat]
	Comment 47 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fbe1cc85a7e6
https://hg.mozilla.org/mozilla-central/rev/ee83d585ff9d
https://hg.mozilla.org/mozilla-central/rev/d6663520be22
https://hg.mozilla.org/mozilla-central/rev/253cccc6907d
https://hg.mozilla.org/mozilla-central/rev/bee484c1a00d
https://hg.mozilla.org/mozilla-central/rev/f97c038c911d
https://hg.mozilla.org/mozilla-central/rev/e729283e0726
https://hg.mozilla.org/mozilla-central/rev/f0e7ef6f7289
https://hg.mozilla.org/mozilla-central/rev/d8fd0a41c62f
https://hg.mozilla.org/mozilla-central/rev/d008348cd4f4

	Nicholas Nethercote [inactive] Assignee
	Comment 48 • 8 years ago

tnikkel: I accidentally landed this patch that you haven't yet given r+ to. Sorry!

Here is the patch as landed, with nsMozIconURI::Equals() updated to always set |result|. If you're not happy with the patch as is I can back it out or fix it via a follow-up, whichever you prefer.

	Timothy Nikkel (:tnikkel)
	Comment 49 • 8 years ago

No problem, followup is fine. Sorry for not getting to it sooner.