1297961 - Introduce nsIURI::GetSpecOrDefault()

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1297300 is about adding [must_use] to nsIURI::GetSpec() so you can't forget to check its return value. There are several hundred calls to it missing checks so it's a big job.

One big group of them is where the resulting spec is printed in a log/warning/error message. In this case, if GetSpec() fails it's simpler and better to continue with some kind of failure-indicating string than to do some kind of early return. To support this, I will add a C++ helper function. This will make the relevant call sites more concise, too.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Introduce nsIRUI::GetSpecOr()

This function is an infallible alternative to nsIURI::GetSpec(). It's useful
when it's appropriate to handle a GetSpec() failure with a failure string, e.g.
for log/warning/error messages. It allows code like this:

  nsAutoCString spec;
  uri->GetSpec(spec);
  printf("uri: %s", spec.get());

to be changed to this:

  printf("uri: %s", uri->GetSpecOr().get());

This introduces a slight behavioural change. Previously, if GetSpec() failed,
an empty string would be used here. Now, "[unknown URI spec]" will be produced
instead (unless GetSpecOr() is passed an argument containing a different string
to use on failure). In most cases this failure string will make for a clearer
log/warning/error message than the empty string.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use nsIRUI::GetSpecOr() for comparisons to fixed URIs

Comment 3

[u408661]

Comment on attachment 8784703 [details] [diff] [review]
(part 1) - Introduce nsIRUI::GetSpecOr()

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

First thing's first: s/nsIRUI/nsIURI/ in the first line of the commit message. :)

Other than that, and a few nits below, the code looks good, but it leaves me wondering two things:

(1) Should we do the same thing for the other getters - especially GetAsciiSpec? I haven't looked at usages there, so it may be a much smaller issue than GetSpec and therefore lower priority.

(2) I don't want to get into a bikeshed here (which is why I'm not making a bigger deal), but should we add "Error" (or similar) to the end of the method name? "GetSpecOr" in the default usage (which is definitely the most common) leaves me hanging every time I read it, and my inner voice goes "Get spec or WHAT?!"

::: dom/xul/XULDocument.cpp
@@ +2539,4 @@
>  
>          MOZ_LOG(gXULLog, LogLevel::Debug,
> +                ("xul: %s loading overlay %s",
> +                uri ? uri->GetSpecOr().get() : "",

nit: indent one more space here & below

::: netwerk/base/nsIURI.idl
@@ +94,5 @@
> +    // An infallible wrapper for GetSpec() that returns aOnFailure if GetSpec()
> +    // fails. It is most useful for creating logging/warning/error messages
> +    // produced for human consumption, and when matching a URI spec against a
> +    // fixed spec such as about:blank.
> +    nsCString GetSpecOr(const char* aOnFailure = "[unknown URI spec]") {

nit: put { on its own line

::: rdf/base/nsRDFXMLDataSource.cpp
@@ +830,2 @@
>        MOZ_LOG(gLog, LogLevel::Debug,
> +             ("rdfxml[%p] flush(%s)", this, mURL->GetSpecOr().get()));

nit: since you're here, fix the indentation :)

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +467,2 @@
>          LOG(("%p: Done fetching offline item %s [status=%x]\n",
> +            this, mURI->GetSpecOr().get(), aStatus));

nit: again, go ahead and fix the indentation while you're here

@@ +1885,5 @@
>      }
>  
>      if (LOG_ENABLED()) {
> +        LOG(("%p: Opening channel for %s", this,
> +            runItem->mURI->GetSpecOr().get()));

nit: indentation

Comment 4

[u408661]

Comment on attachment 8784705 [details] [diff] [review]
(part 2) - Use nsIRUI::GetSpecOr() for comparisons to fixed URIs

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

s/nsIRUI/nsIURI/ in the commit message, and I think we're good to go here (pending resolution on the other patch on this bug).

Comment 5

[Nicholas Nethercote [inactive]]

Thank you for the fast reviews.


> (1) Should we do the same thing for the other getters - especially
> GetAsciiSpec? I haven't looked at usages there, so it may be a much smaller
> issue than GetSpec and therefore lower priority.

Very likely. I'm starting with GetSpec() for reasons explained in bug 1297300 (specifically, bug 1290350 comment 6) and it alone is a big task.

> (2) I don't want to get into a bikeshed here (which is why I'm not making a
> bigger deal), but should we add "Error" (or similar) to the end of the
> method name? "GetSpecOr" in the default usage (which is definitely the most
> common) leaves me hanging every time I read it, and my inner voice goes "Get
> spec or WHAT?!"

It's a fair point :)  I was inspired by Rust's Option type, which has functions like this:

- unwrap_or(x):        unwrap, or use x on failure
- unwrap_or_default(): unwrap, or use the default value of the relevant type on failure
- unwrap_or_else(f):   unwrap, or use the value computed by the closure f on failure

But I was working from memory and what I did doesn't quite match that. In particular, my use of the default argument complicates things... and now I see that I don't really need that default argument, because the only time I'm using it is to provide "" in part 2, but it's obvious that "[unknown URI spec]" won't match against "about:blank" or similar URIs so it's not really necessary.

So I think I will remove the default argument. So that suggests GetSpecOrDefault() might be a better name? It's not immediately what the default value is -- you'd have to look at the function's comments to see that -- but I think it's clearer than the current situation.

Thoughts?

Comment 6

[Nicholas Nethercote [inactive]]

Another possibility is to have two functions:

- GetSpecOrDefault() - uses "[unknown URI spec]" on failure
- GetSpecOr(const char* aFallback) - uses aFallback on failure

And no use of default parameters. Then we could continue to use GetSpecOr("") in part 2, if you think that's important.

Comment 7

[u408661]

I think GetSpecOrDefault() is the way to go here. If we get to the point where we need to add custom/different output to something in the GetSpecOr* family, we can add that kind of function later.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Introduce nsURI::GetSpecOr()

This patch uses the name GetSpecOrDefault() and removes the default argument.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use nsIURI::GetSpecOr() for comparisons to fixed URIs

Might as well re-review this one too.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Introduce nsURI::GetSpecOrDefault()

I forgot to update the commit message for the new name.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 1b) - More GetSpecOrDefault() additions

I found a few more logging cases where GetSpecOrDefault() can be used. I will
fold this into part 1 before landing.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use nsIURI::GetSpecOrDefault() for comparisons to fixed URIs

I updated this patch's commit message too.

Comment 13

[u408661]

Comment on attachment 8785796 [details] [diff] [review]
(part 1) - Introduce nsURI::GetSpecOrDefault()

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

LGTM, just a few nits.

::: dom/plugins/base/nsPluginHost.cpp
@@ +952,3 @@
>    MOZ_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_NORMAL,
> +          ("nsPluginHost::TrySetupPluginInstance Begin mime=%s, owner=%p, url=%s\n",
> +          PromiseFlatCString(aMimeType).get(), aOwner,

nit: indentation off by one here & next line

::: dom/plugins/base/nsPluginStreamListenerPeer.cpp
@@ +313,3 @@
>    MOZ_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_NORMAL,
> +          ("nsPluginStreamListenerPeer::Initialize instance=%p, url=%s\n",
> +          aInstance, aURL ? aURL->GetSpecOrDefault().get() : ""));

nit: indentation off by one

::: dom/security/nsCSPContext.cpp
@@ +109,5 @@
>                           int16_t*            outDecision)
>  {
>    if (CSPCONTEXTLOGENABLED()) {
> +    CSPCONTEXTLOG(("nsCSPContext::ShouldLoad, aContentLocation: %s",
> +                  aContentLocation->GetSpecOrDefault().get()));

nit: indentation

@@ +1228,5 @@
>        uriClone->SetUserPass(EmptyCString());
>  
>        if (CSPCONTEXTLOGENABLED()) {
> +        CSPCONTEXTLOG(("nsCSPContext::PermitsAncestry, found ancestor: %s",
> +                      uriClone->GetSpecOrDefault().get()));

nit: indentation

@@ +1247,5 @@
>  
>    for (uint32_t a = 0; a < ancestorsArray.Length(); a++) {
>      if (CSPCONTEXTLOGENABLED()) {
> +      CSPCONTEXTLOG(("nsCSPContext::PermitsAncestry, checking ancestor: %s",
> +                    ancestorsArray[a]->GetSpecOrDefault().get()));

nit: indentation

@@ +1296,2 @@
>        CSPCONTEXTLOG(("nsCSPContext::Permits, aUri: %s, aDir: %d, isAllowed: %s",
> +                    aURI->GetSpecOrDefault().get(), aDir,

nit: indentation (may as well fix it while you're here, since it was already off)

::: dom/security/nsCSPParser.cpp
@@ +1303,5 @@
>    if (CSPPARSERLOGENABLED()) {
>      CSPPARSERLOG(("nsCSPParser::parseContentSecurityPolicy, policy: %s",
>                   NS_ConvertUTF16toUTF8(aPolicyString).get()));
> +    CSPPARSERLOG(("nsCSPParser::parseContentSecurityPolicy, selfURI: %s",
> +                 aSelfURI->GetSpecOrDefault().get()));

nit: indentation

::: dom/security/nsCSPService.cpp
@@ +120,2 @@
>      MOZ_LOG(gCspPRLog, LogLevel::Debug,
> +           ("CSPService::ShouldLoad called for %s",

nit: indentation (may as well fix it while you're here)

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +72,5 @@
>    {
>      nsCOMPtr<nsIURI> uri;
>      mChannel->GetURI(getter_AddRefs(uri));
> +    CHANNELWRAPPERLOG(("nsSecCheckWrapChannel::nsSecCheckWrapChannel [%p] (%s)",
> +                      this, uri ? uri->GetSpecOrDefault().get() : ""));

nit: indentation

::: rdf/base/nsRDFXMLDataSource.cpp
@@ +970,2 @@
>        MOZ_LOG(gLog, LogLevel::Debug,
> +             ("rdfxml[%p] begin-load(%s)", this,

nit: indentation (fix while you're here)

@@ +993,2 @@
>        MOZ_LOG(gLog, LogLevel::Debug,
> +             ("rdfxml[%p] interrupt(%s)", this,

nit: indentation

@@ +1015,2 @@
>        MOZ_LOG(gLog, LogLevel::Debug,
> +             ("rdfxml[%p] resume(%s)", this,

nit: indentation

@@ +1037,2 @@
>        MOZ_LOG(gLog, LogLevel::Debug,
> +             ("rdfxml[%p] end-load(%s)", this,

nit: indentation

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b516e1d6e461
(part 1) - Introduce nsURI::GetSpecOrDefault(). r=hurley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b7827cca83
(part 2) - Use nsIURI::GetSpecOrDefault() for comparisons to fixed URIs. r=hurley.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b516e1d6e461
https://hg.mozilla.org/mozilla-central/rev/d8b7827cca83