Closed Bug 1433958 Opened 6 years ago Closed 6 years ago

Change code that uses nsIURI setters to use nsIURIMutator

Categories

(Core :: Networking, defect, P2)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(18 files)

Bug 1433958 - Change code that uses nsIURI.hostPort to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that uses nsIURI.setHostAndPort() to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Make NS_MutateURI methods warn immediately after calling method that fails
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.pathQueryRef to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.filePath to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that calls nsIURI.setQueryWithEncoding to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.userPass to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change URLWorker to use NS_MutateURI
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.username to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.password to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Don't fail when setting an empty password if the username is already empty
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.ref to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.query to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.port to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.host to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - If setting the host succeeds, SetHostPort should not return an error code
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1433958 - Change code that sets nsIURI.scheme to use nsIURIMutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4012
52 bytes, text/x-github-pull-request
Details | Review 
      No description provided.
Comment on attachment 8946511 [details]
Bug 1433958 - Change code that sets nsIURI.password to use nsIURIMutator

https://reviewboard.mozilla.org/r/216530/#review222212


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/streamconv/converters/nsFTPDirListingConv.cpp:196
(Diff revision 1)
>      nsAutoCString pw;
>      nsAutoCString spec;
>      uri->GetPassword(pw);
>      if (!pw.IsEmpty()) {
> -         rv = uri->SetPassword(EmptyCString());
> -         if (NS_FAILED(rv)) return rv;
> +         nsCOMPtr<nsIURI> noPassURI;
> +         rv = NS_MutateURI(uri)

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment on attachment 8946511 [details]
Bug 1433958 - Change code that sets nsIURI.password to use nsIURIMutator

https://reviewboard.mozilla.org/r/216530/#review222226


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/streamconv/converters/nsFTPDirListingConv.cpp:196
(Diff revision 2)
>      nsAutoCString pw;
>      nsAutoCString spec;
>      uri->GetPassword(pw);
>      if (!pw.IsEmpty()) {
> -         rv = uri->SetPassword(EmptyCString());
> -         if (NS_FAILED(rv)) return rv;
> +         nsCOMPtr<nsIURI> noPassURI;
> +         rv = NS_MutateURI(uri)

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment on attachment 8946502 [details]
Bug 1433958 - Change code that uses nsIURI.hostPort to use nsIURIMutator

https://reviewboard.mozilla.org/r/216512/#review222332
Attachment #8946502 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946503 [details]
Bug 1433958 - Change code that uses nsIURI.setHostAndPort() to use nsIURIMutator

https://reviewboard.mozilla.org/r/216514/#review222334

::: dom/html/nsHTMLDocument.cpp:943
(Diff revision 2)
>  
> -  nsCOMPtr<nsIURI> newURI;
> -  nsresult rv = uri->Clone(getter_AddRefs(newURI));
> -  if (NS_FAILED(rv)) {
> -    return nullptr;
> -  }
> +  nsresult rv;
> +  rv = NS_MutateURI(uri)
> +         .SetUserPass(EmptyCString())
> +         .SetPort(-1) // we want to reset the port number if needed.
> +         .SetHostPort(aHostString)

hmm... if SetHostPort() accepts a single argument - "host[:port]" string, then why do you need to do SetPort(-1) ?

I would expect that when URI.port == 100 (for an http: URL) then:

setHost("foo") -> URI.host == "foo", URI.port == 100

setHostPort("foo") -> URI.host == "foo", URI.port == -1

setHostPort("foo:999") -> URI.host == "foo", URI.port == 999


So, I don't think you should need SetPort(-1).
Comment on attachment 8946504 [details]
Bug 1433958 - Make NS_MutateURI methods warn immediately after calling method that fails

https://reviewboard.mozilla.org/r/216516/#review222336

agree!
Attachment #8946504 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946503 [details]
Bug 1433958 - Change code that uses nsIURI.setHostAndPort() to use nsIURIMutator

https://reviewboard.mozilla.org/r/216514/#review222338

::: dom/html/nsHTMLDocument.cpp:943
(Diff revision 2)
>  
> -  nsCOMPtr<nsIURI> newURI;
> -  nsresult rv = uri->Clone(getter_AddRefs(newURI));
> -  if (NS_FAILED(rv)) {
> -    return nullptr;
> -  }
> +  nsresult rv;
> +  rv = NS_MutateURI(uri)
> +         .SetUserPass(EmptyCString())
> +         .SetPort(-1) // we want to reset the port number if needed.
> +         .SetHostPort(aHostString)

This was SetHostAndPort - which always resets the port if none is given. This is exactly what the implementation of nsStandardURL::SetHostAndPort does.
Depends on: 1420714
No longer depends on: 1420714
Comment on attachment 8946505 [details]
Bug 1433958 - Change code that sets nsIURI.pathQueryRef to use nsIURIMutator

https://reviewboard.mozilla.org/r/216518/#review222360

::: dom/base/nsGlobalWindowOuter.cpp:5805
(Diff revision 2)
>  
> -    if (NS_FAILED(originURI->SetUserPass(EmptyCString())) ||
> -        NS_FAILED(originURI->SetPathQueryRef(EmptyCString()))) {
> +    nsresult rv = NS_MutateURI(originURI)
> +                    .SetUserPass(EmptyCString())
> +                    .SetPathQueryRef(EmptyCString())
> +                    .Finalize(originURI);
> +    if (NS_FAILED(rv)) {

there is NS_NewURI and then NS_MutateURI on the result (we clone).  could this be changed so that we construct the originURI with the mutator?

::: dom/html/HTMLFormSubmission.h:88
(Diff revision 2)
>     * stream that will be submitted.  Subclasses *must* implement this.
>     *
> -   * @param aURI the URI being submitted to [INOUT]
> +   * @param aURI the URI being submitted to [IN]
>     * @param aPostDataStream a data stream for POST data [OUT]
>     * @param aPostDataStreamLength a data stream for POST data length [OUT]
> +   * @param aOutURI the resulting URI [OUT]

maybe add, that it can be the same as aURI

::: dom/html/HTMLFormSubmission.cpp:773
(Diff revision 2)
>      path += NS_LITERAL_CSTRING("&force-plain-text=Y&body=") + escapedBody;
>  
> -    rv = aURI->SetPathQueryRef(path);
> -
> +    return NS_MutateURI(aURI)
> +             .SetPathQueryRef(path)
> +             .Finalize(aOutURI);
>    } else {

else after return!

::: netwerk/protocol/ftp/nsFTPChannel.h:51
(Diff revision 2)
>      {
>          SetURI(uri);
>      }
>  
> +    void UpdateURI(nsIURI *aURI) {
> +        mURI = aURI;

assert thread if possible please

::: netwerk/protocol/http/HttpBaseChannel.cpp:1893
(Diff revision 2)
>        case 2: // scheme+host+port+/
>          spec.AppendLiteral("/");
>          // This nukes any query/ref present as well in the case of nsStandardURL
> -        rv = clone->SetPathQueryRef(EmptyCString());
> +        rv = NS_MutateURI(clone)
> +               .SetPathQueryRef(EmptyCString())
> +               .Finalize(clone);

lot of mutations (=clones)... would be nice (a followup maybe?) to leave the mutator alive until we are done with all the changes

it seems doable as all the getters can be called on the source (being mutated) uri.

::: netwerk/streamconv/converters/nsIndexedToHTML.cpp:254
(Diff revision 2)
>          rv = uri->GetPathQueryRef(path);
>          if (NS_FAILED(rv)) return rv;
>          if (baseUri.Last() != '/') {
>              baseUri.Append('/');
>              path.Append('/');
> -            uri->SetPathQueryRef(path);
> +            mozilla::Unused << NS_MutateURI(uri)

so, this was modifying the URI on the channel!  hence, this is not a small change.  can you verify this was actually wrong before your patch and that there are no regressions?
Comment on attachment 8946506 [details]
Bug 1433958 - Change code that sets nsIURI.filePath to use nsIURIMutator

https://reviewboard.mozilla.org/r/216520/#review222372

::: dom/base/Link.cpp:509
(Diff revision 2)
>  }
>  
>  void
>  Link::SetPathname(const nsAString &aPathname)
>  {
>    nsCOMPtr<nsIURI> uri(GetURIToMutate());

GetURI()?

(btw, can GetURIToMutate be removed?)

::: toolkit/content/contentAreaUtils.js:1101
(Diff revision 2)
>      return ""; // temporary fix for bug 120327
>  
>    // First try the extension from the filename
> -  const stdURLContractID = "@mozilla.org/network/standard-url;1";
> -  const stdURLIID = Components.interfaces.nsIURL;
> -  var url = Components.classes[stdURLContractID].createInstance(stdURLIID);
> +  var url = Components.classes["@mozilla.org/network/standard-url-mutator;1"]
> +                      .createInstance(Components.interfaces.nsIURIMutator)
> +                      .setSpec("http://example.com") // construct the URL

hmm.. why can't you omit this step?
Attachment #8946506 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946507 [details]
Bug 1433958 - Change code that calls nsIURI.setQueryWithEncoding to use nsIURIMutator

https://reviewboard.mozilla.org/r/216522/#review222376
Attachment #8946507 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946508 [details]
Bug 1433958 - Change code that sets nsIURI.userPass to use nsIURIMutator

https://reviewboard.mozilla.org/r/216524/#review222382

all comments are more for optimization sake, not functional.  so, up to you to file (and track) a followup to potentially fix it, or update this patch before landing (I might review then?)

::: docshell/base/nsDefaultURIFixup.cpp:94
(Diff revision 2)
>        return NS_ERROR_FAILURE;
>      }
>  
>      rv = NS_NewURI(getter_AddRefs(uri),
>                     Substring(path, slashIndex + 1, pathLength - slashIndex - 1));
>      NS_ENSURE_SUCCESS(rv, rv);

would be nice to create with a mutator and keep it (to save cloning later), but it's optional

::: docshell/base/nsDefaultURIFixup.cpp:97
(Diff revision 2)
>      rv = NS_NewURI(getter_AddRefs(uri),
>                     Substring(path, slashIndex + 1, pathLength - slashIndex - 1));
>      NS_ENSURE_SUCCESS(rv, rv);
>    } else {
> -    // clone the URI so zapping user:pass doesn't change the original
> +    // clone the URI so changing the returned URI doesn't modify the original
>      nsresult rv = aURI->Clone(getter_AddRefs(uri));

can you do this only when you don't mutate to save a clone?

::: dom/security/nsCSPContext.cpp:1459
(Diff revision 2)
>  
>      currentURI = doc->GetDocumentURI();
>  
>      if (currentURI) {
>        // delete the userpass from the URI.
>        rv = currentURI->CloneIgnoringRef(getter_AddRefs(uriClone));

hmm.. another place where we clone twice... do you plan a method on the mutator interface(es) to cut Ref?

like:

NS_MutateURI(currentURI).RemoveRef().SetUserPass("").Finalize(uriClone);

::: dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp:751
(Diff revision 2)
>          nsresult rv = NS_NewURI(getter_AddRefs(newURI), oldCValue,
>                                  mParent->GetCharacterSet(), relativeURI);
>          if (NS_SUCCEEDED(rv) && newURI) {
> -            newURI->SetUserPass(EmptyCString());
> +            Unused << NS_MutateURI(newURI)
> +                        .SetUserPass(EmptyCString())
> +                        .Finalize(newURI);

here again

::: dom/webbrowserpersist/nsWebBrowserPersist.cpp:2581
(Diff revision 2)
>      }
>  
>      // remove username/password if present
> -    fileAsURI->SetUserPass(EmptyCString());
> +    Unused << NS_MutateURI(fileAsURI)
> +                .SetUserPass(EmptyCString())
> +                .Finalize(fileAsURI);

up to 3 clones (AppendPathToURI mutates as well..)

::: dom/xhr/XMLHttpRequestMainThread.cpp
(Diff revision 2)
> -    }
> -    userpass.AppendLiteral(":");
> +                  .SetPassword(NS_ConvertUTF16toUTF8(aPassword))
> +                  .Finalize(parsedURL);
> -    if (!aPassword.IsVoid()) {
> -      AppendUTF16toUTF8(aPassword, userpass);
>      }
> -    parsedURL->SetUserPass(userpass);

so (replying to the commit message here as well) - have changed the way how setUserPass behaves now?  before your patches: URI->SetUserPass(":") just removed the user:pass portion?  and now NS_MutateURI(uri).SetUserPass(":").Finalize(..) throws?  maybe it's OK, if this is the only use you had to fix..

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:490
(Diff revision 2)
>    nsresult rv = aUri->CloneIgnoringRef(getter_AddRefs(clone));
>    nsCOMPtr<nsIURL> url(do_QueryInterface(clone));
>    if (url) {
> -    rv = url->SetQuery(EmptyCString());
> +    rv = NS_MutateURI(url)
> +           .SetQuery(EmptyCString())
> +           .SetRef(EmptyCString())

- two clones
- why SetRef("") when cloned "IgnoringRef" ? (probably an old bug)
Attachment #8946508 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946509 [details]
Bug 1433958 - Change URLWorker to use NS_MutateURI

https://reviewboard.mozilla.org/r/216526/#review222386

::: dom/url/URLWorker.cpp:873
(Diff revision 2)
>  }
>  
>  void
>  URLWorker::SetUsername(const nsAString& aUsername, ErrorResult& aRv)
>  {
>    STDURL_SETTER(aUsername, SetUsername);

is this used on more than one thread?  I know this is not thread safe even before the patch, but I think we should add thread assertions (in followups) and in case of a failure definitely fix them (add a mutex)
Attachment #8946509 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946510 [details]
Bug 1433958 - Change code that sets nsIURI.username to use nsIURIMutator

https://reviewboard.mozilla.org/r/216528/#review222390
Attachment #8946510 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946511 [details]
Bug 1433958 - Change code that sets nsIURI.password to use nsIURIMutator

https://reviewboard.mozilla.org/r/216530/#review222392
Attachment #8946511 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946512 [details]
Bug 1433958 - Don't fail when setting an empty password if the username is already empty

https://reviewboard.mozilla.org/r/216532/#review222394

::: netwerk/base/nsStandardURL.cpp:1856
(Diff revision 2)
>  
>      return NS_OK;
>  }
>  
>  NS_IMETHODIMP
>  nsStandardURL::SetPassword(const nsACString &input)

this is still a public method?  (sorry, getting a bit lost what has been made private and what not)

::: netwerk/base/nsStandardURL.cpp:1874
(Diff revision 2)
>      }
>      if (mUsername.mLen <= 0) {
> +        if (password.IsEmpty()) {
> +            return NS_OK;
> +        }
>          NS_WARNING("cannot set password without existing username");

is there a possible code path that could bring us to having a spec like:

"http://:secretpwd@example.com/"

because then calling setPassword("") on such URL would leave the password set - probably leaking the password to the world!

I think we should make sure (not just assert, please) that mPassword is made "" when input == "".
Comment on attachment 8946513 [details]
Bug 1433958 - Change code that sets nsIURI.ref to use nsIURIMutator

https://reviewboard.mozilla.org/r/216534/#review222396

::: dom/xbl/nsXBLPrototypeBinding.cpp:126
(Diff revision 2)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // The binding URI might be an immutable URI (e.g. for about: URIs). In that case,
>    // we'll fail in SetRef below, but that doesn't matter much for now.
>    if (aFirstBinding) {
>      rv = mBindingURI->Clone(getter_AddRefs(mAlternateBindingURI));

hmm.. as you mutate a line below, you could just do mAlternateBindingURI = mBindingURI

::: layout/style/nsCSSValue.cpp:3017
(Diff revision 2)
>  
> +    nsresult rv = NS_MutateURI(aURI)
> +                    .SetRef(ref)
> +                    .Finalize(result);
> +    if (NS_FAILED(rv)) {
> +      // If setting the ref failed, just return a clone.

// because we are ignoring SetRef error here
Attachment #8946513 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946514 [details]
Bug 1433958 - Change code that sets nsIURI.query to use nsIURIMutator

https://reviewboard.mozilla.org/r/216536/#review222398

::: dom/cache/DBAction.cpp:202
(Diff revision 2)
>    RefPtr<nsFileProtocolHandler> handler = new nsFileProtocolHandler();
>    rv = handler->Init();
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
>    nsCOMPtr<nsIURI> uri;
>    rv = handler->NewFileURI(dbFile, getter_AddRefs(uri));

could we create this using a mutator so it can be used below?

database openings are usually on critical paths..

::: dom/indexedDB/ActorsParent.cpp:4204
(Diff revision 2)
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  
>    nsCOMPtr<nsIURI> uri;
>    rv = fileHandler->NewFileURI(aDatabaseFile, getter_AddRefs(uri));

potentially here as well

::: netwerk/protocol/file/nsFileChannel.cpp:303
(Diff revision 2)
>        NS_SUCCEEDED(file->GetNativeTarget(fileTarget)) &&
>        NS_SUCCEEDED(NS_NewNativeLocalFile(fileTarget, true,
>                                           getter_AddRefs(resolvedFile))) &&
>  #endif
>        NS_SUCCEEDED(NS_NewFileURI(getter_AddRefs(targetURI),
>                                   resolvedFile, nullptr))) {

would be nice to get a mutator here, but not that critical..

::: services/sync/modules/record.js:651
(Diff revision 2)
>        args.push("offset=" + encodeURIComponent(this._offset));
>      }
>  
> -    this.uri.query = (args.length > 0) ? "?" + args.join("&") : "";
> +    this.uri = this.uri.mutate()
> +                       .setQuery((args.length > 0) ? "?" + args.join("&") : "")
> +                       .finalize();

hmm.. one potential (very theoretical, tho) problem could be when someone takes a ref to this.uri before this mutation and expects the reference to be updated after this mutation.

however, this might be an overkill to check all consumers...  just raising for awareness here...

this however applies to any mutations made on such (publicly accessible) member, not just js...
Attachment #8946514 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946515 [details]
Bug 1433958 - Change code that sets nsIURI.port to use nsIURIMutator

https://reviewboard.mozilla.org/r/216538/#review222410

::: dom/base/Link.cpp:556
(Diff revision 2)
>  }
>  
>  void
>  Link::SetPort(const nsAString &aPort)
>  {
>    nsCOMPtr<nsIURI> uri(GetURIToMutate());

GetURI()?

::: dom/base/Location.cpp:658
(Diff revision 2)
>      aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
>      return;
>    }
>  
>    nsCOMPtr<nsIURI> uri;
>    aRv = GetWritableURI(getter_AddRefs(uri));

GetURI()?

::: netwerk/test/unit/test_bug388281.js:12
(Diff revision 2)
>    Assert.equal(uri.hostPort, "foo.com:90");
>  
>    uri = ios.newURI("http://foo.com:10/file.txt");
> -  uri.port = 500;
> +  uri = uri.mutate().setPort(500).finalize();
>    Assert.equal(uri.hostPort, "foo.com:500");
>    

maybe remove this trail ws?
Attachment #8946515 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946516 [details]
Bug 1433958 - Change code that sets nsIURI.host to use nsIURIMutator

https://reviewboard.mozilla.org/r/216540/#review222416

::: caps/nsScriptSecurityManager.cpp:540
(Diff revision 2)
>  EqualOrSubdomain(nsIURI* aProbeArg, nsIURI* aBase)
>  {
>      // Make a clone of the incoming URI, because we're going to mutate it.
>      nsCOMPtr<nsIURI> probe;
>      nsresult rv = aProbeArg->Clone(getter_AddRefs(probe));
>      NS_ENSURE_SUCCESS(rv, false);

instead of the clone, I believe you can just:

probe = aProbeArg;

::: dom/base/Link.cpp:504
(Diff revision 2)
>  }
>  
>  void
>  Link::SetHostname(const nsAString &aHostname)
>  {
>    nsCOMPtr<nsIURI> uri(GetURIToMutate());

getURI?

::: extensions/cookie/nsPermissionManager.cpp:661
(Diff revision 2)
>        nsAutoCString uriSpec;
>        rv = child->GetUri(uriSpec);
>        if (NS_WARN_IF(NS_FAILED(rv))) continue;
>  
>        nsCOMPtr<nsIURI> uri;
>        rv = NS_NewURI(getter_AddRefs(uri), uriSpec);

could we create by mutation?
Attachment #8946516 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946517 [details]
Bug 1433958 - If setting the host succeeds, SetHostPort should not return an error code

https://reviewboard.mozilla.org/r/216542/#review222420

r+ in this case with comments definitely addressed

::: commit-message-ae0ae:9
(Diff revision 2)
> +url.host = "host:" // port missing
> +url.host = "host:65536" // port too big
> +url.host = "host:bla" // invalid port
> +that the hostname will be set, but the port will be left unchanged.
> +Since DOM APIs are the only consumers for SetHostPort it means we can change this behaviour to match the WPT expectations.
> +As such, SetHostPort will return NS_OK if setting the host succeded, and will ignore if setting the port failed.

this change MUST be reflected in the related idl file!!

::: netwerk/base/nsStandardURL.cpp:1988
(Diff revision 2)
>          if (iter != end) {
>              nsCString portStr(Substring(iter, end));
>              nsresult rv;
>              int32_t port = portStr.ToInteger(&rv);
>              if (NS_SUCCEEDED(rv)) {
>                  rv = SetPort(port);

Unused <<

::: netwerk/base/nsStandardURL.cpp:1990
(Diff revision 2)
>              nsresult rv;
>              int32_t port = portStr.ToInteger(&rv);
>              if (NS_SUCCEEDED(rv)) {
>                  rv = SetPort(port);
> -                NS_ENSURE_SUCCESS(rv, rv);
> +                return NS_OK;
>              } else {

else after return

::: netwerk/base/nsStandardURL.cpp:1996
(Diff revision 2)
>                  // Failure parsing port number
> -                return NS_ERROR_MALFORMED_URI;
> +                return NS_OK;
>              }
>          } else {
>              // port number is missing
> -            return NS_ERROR_MALFORMED_URI;
> +            return NS_OK;

I think those return NS_OK can be reduced to only one?

::: netwerk/test/unit/test_standardurl.js
(Diff revision 2)
>    Assert.throws(() => { url = url.mutate().setHostPort("[2001:1").finalize(); }, "bad IPv6 address");
>    Assert.throws(() => { url = url.mutate().setHostPort("2001]:1").finalize(); }, "bad IPv6 address");
>    Assert.throws(() => { url = url.mutate().setHostPort("2001:1]").finalize(); }, "bad IPv6 address");
>    Assert.throws(() => { url = url.mutate().setHostPort("").finalize(); }, "Empty hostPort should fail");
> -  Assert.throws(() => { url = url.mutate().setHostPort("[2001::1]:").finalize(); }, "missing port number");
> -  Assert.throws(() => { url = url.mutate().setHostPort("[2001::1]:bad").finalize(); }, "bad port number");

maybe convert them to a "silently ignored" check type of tests?
Attachment #8946517 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946518 [details]
Bug 1433958 - Change code that sets nsIURI.scheme to use nsIURIMutator

https://reviewboard.mozilla.org/r/216544/#review222422

::: dom/base/Location.cpp:729
(Diff revision 2)
>      aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
>      return;
>    }
>  
>    nsCOMPtr<nsIURI> uri;
>    aRv = GetWritableURI(getter_AddRefs(uri));

GetURI?

::: netwerk/test/unit/test_URIs.js:526
(Diff revision 2)
> -
> -    do_info("testing that setting '" + aProperty +
> -            "' on immutable URI '" + aTest.spec + "' will throw");
> -    Assert.ok(threw);
> -  });
>  }

interesting test :)  could this be just removed?

::: netwerk/test/unit/test_URIs2.js:627
(Diff revision 2)
> -
> -    do_info("testing that setting '" + aProperty +
> -            "' on immutable URI '" + aTest.spec + "' will throw");
> -    Assert.ok(threw);
> -  });
>  }

ditto

::: security/manager/pki/resources/content/exceptionDialog.js:136
(Diff revision 2)
> -    uri.scheme = "https";
> +    uri = uri.mutate().setScheme("https").finalize();
>    }
>  
>    if (uri.port == -1) {
>      uri = uri.mutate().setPort(443).finalize();
>    }

could we merge these two mutations?
Attachment #8946518 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946503 [details]
Bug 1433958 - Change code that uses nsIURI.setHostAndPort() to use nsIURIMutator

https://reviewboard.mozilla.org/r/216514/#review222444

r+ based on https://reviewboard.mozilla.org/r/216514/#review222338

I missed that!
Attachment #8946503 - Flags: review?(honzab.moz) → review+
Comment on attachment 8946505 [details]
Bug 1433958 - Change code that sets nsIURI.pathQueryRef to use nsIURIMutator

https://reviewboard.mozilla.org/r/216518/#review222446

r- to track open issues
Attachment #8946505 - Flags: review?(honzab.moz) → review-
Comment on attachment 8946512 [details]
Bug 1433958 - Don't fail when setting an empty password if the username is already empty

https://reviewboard.mozilla.org/r/216532/#review222448

r- to track open issues
Attachment #8946512 - Flags: review?(honzab.moz) → review-
Comment on attachment 8946505 [details]
Bug 1433958 - Change code that sets nsIURI.pathQueryRef to use nsIURIMutator

https://reviewboard.mozilla.org/r/216518/#review222360

> there is NS_NewURI and then NS_MutateURI on the result (we clone).  could this be changed so that we construct the originURI with the mutator?

At the moment you can't. NS_NewURI creates a new URI via the protocol handler, depending on the scheme.
I filed bug 1434507 to maybe improve this pattern.

> so, this was modifying the URI on the channel!  hence, this is not a small change.  can you verify this was actually wrong before your patch and that there are no regressions?

The try run didn't show any failures in this area.
I tested locally with resource://gre/res/html/ and resource://gre/res/html and got the same behaviour as in the current nightly.
Comment on attachment 8946505 [details]
Bug 1433958 - Change code that sets nsIURI.pathQueryRef to use nsIURIMutator

https://reviewboard.mozilla.org/r/216518/#review222688
Attachment #8946505 - Flags: review- → review+
Comment on attachment 8946506 [details]
Bug 1433958 - Change code that sets nsIURI.filePath to use nsIURIMutator

https://reviewboard.mozilla.org/r/216520/#review222880

::: dom/base/Link.cpp:509
(Diff revision 2)
>  }
>  
>  void
>  Link::SetPathname(const nsAString &aPathname)
>  {
>    nsCOMPtr<nsIURI> uri(GetURIToMutate());

You're right. We can probably remove it. I'll try add it to the patch that removes nsIURI setters.

::: toolkit/content/contentAreaUtils.js:1101
(Diff revision 2)
>      return ""; // temporary fix for bug 120327
>  
>    // First try the extension from the filename
> -  const stdURLContractID = "@mozilla.org/network/standard-url;1";
> -  const stdURLIID = Components.interfaces.nsIURL;
> -  var url = Components.classes[stdURLContractID].createInstance(stdURLIID);
> +  var url = Components.classes["@mozilla.org/network/standard-url-mutator;1"]
> +                      .createInstance(Components.interfaces.nsIURIMutator)
> +                      .setSpec("http://example.com") // construct the URL

Because otherwise the mutator doesn't have a URI, and the call would fail.
I'm not even sure this code worked before.
Comment on attachment 8946508 [details]
Bug 1433958 - Change code that sets nsIURI.userPass to use nsIURIMutator

https://reviewboard.mozilla.org/r/216524/#review222382

> would be nice to create with a mutator and keep it (to save cloning later), but it's optional

I'll just not clone the URI here, since we normally mutate it anyway.
I'll leave the rest for bug 1434507 (not mutating after NS_NewURI)

> hmm.. another place where we clone twice... do you plan a method on the mutator interface(es) to cut Ref?
> 
> like:
> 
> NS_MutateURI(currentURI).RemoveRef().SetUserPass("").Finalize(uriClone);

Fair enough. .SetRef(EmptyCString()) does the job. I'll fix it.

> here again

A bit harder to fix here. We don't have an API to resolve directly to an nsIURI yet. I'll leave it as a follow-up.

> up to 3 clones (AppendPathToURI mutates as well..)

True. But I won't bother with this right now. This code is only run when saving a webpage, so it shouldn't be a major performance regression.

> so (replying to the commit message here as well) - have changed the way how setUserPass behaves now?  before your patches: URI->SetUserPass(":") just removed the user:pass portion?  and now NS_MutateURI(uri).SetUserPass(":").Finalize(..) throws?  maybe it's OK, if this is the only use you had to fix..

I made the result be unused, so it doesn't throw.
I tracked this down because there were a lot of console warnings from the mutator - SetUserPass(":") was always failing.

> - two clones
> - why SetRef("") when cloned "IgnoringRef" ? (probably an old bug)

removed the unnecessary clone.
Comment on attachment 8946509 [details]
Bug 1433958 - Change URLWorker to use NS_MutateURI

https://reviewboard.mozilla.org/r/216526/#review222386

> is this used on more than one thread?  I know this is not thread safe even before the patch, but I think we should add thread assertions (in followups) and in case of a failure definitely fix them (add a mutex)

mStdURL is only used on the worker thread. There should be no thread-safety issues here.
Comment on attachment 8946512 [details]
Bug 1433958 - Don't fail when setting an empty password if the username is already empty

https://reviewboard.mozilla.org/r/216532/#review222394

> this is still a public method?  (sorry, getting a bit lost what has been made private and what not)

Still public. Apart from setSpecInternal (which is private). I make all the other nsIURI setters private in bug 1434163

> is there a possible code path that could bring us to having a spec like:
> 
> "http://:secretpwd@example.com/"
> 
> because then calling setPassword("") on such URL would leave the password set - probably leaking the password to the world!
> 
> I think we should make sure (not just assert, please) that mPassword is made "" when input == "".

This is a preexisting issue. Without my patch, if we get into this situation, it returns an error code, but still doesn't clear the password. I filed bug 1439632 to do this properly.
Comment on attachment 8946514 [details]
Bug 1433958 - Change code that sets nsIURI.query to use nsIURIMutator

https://reviewboard.mozilla.org/r/216536/#review222398

> could we create this using a mutator so it can be used below?
> 
> database openings are usually on critical paths..

Filed bug 1440191 as a followup.

> hmm.. one potential (very theoretical, tho) problem could be when someone takes a ref to this.uri before this mutation and expects the reference to be updated after this mutation.
> 
> however, this might be an overkill to check all consumers...  just raising for awareness here...
> 
> this however applies to any mutations made on such (publicly accessible) member, not just js...

I agree. This is the reason for 1436589, I think. I'm not going to check all consumers unless there's a regression.
Comment on attachment 8946516 [details]
Bug 1433958 - Change code that sets nsIURI.host to use nsIURIMutator

https://reviewboard.mozilla.org/r/216540/#review222416

> could we create by mutation?

Depends on bug 1434507
Hi Valentin, what's the plan here? This bug switched to the new "scheme of things", but when will you discontinue the old? Is there a bug for that? Will there be "internal" methods coming?

Looking at SetPathQueryRef() or |xx.pathQueryRef = | in JS, it has a hole lot of call sites already, I haven't looked at the other ones.
(In reply to Jorg K (GMT+1) from comment #70)
> Hi Valentin, what's the plan here? This bug switched to the new "scheme of
> things", but when will you discontinue the old? Is there a bug for that?
> Will there be "internal" methods coming?
> 
> Looking at SetPathQueryRef() or |xx.pathQueryRef = | in JS, it has a hole
> lot of call sites already, I haven't looked at the other ones.

Bug 1434163 will make the attributes readonly, and will also remove nsIURI.setSpecInternal.
I hope to land it really soon to prevent new code from using this API.
Comment on attachment 8946512 [details]
Bug 1433958 - Don't fail when setting an empty password if the username is already empty

https://reviewboard.mozilla.org/r/216532/#review229108
Attachment #8946512 - Flags: review?(honzab.moz) → review+
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9438d4a380a2
Change code that uses nsIURI.hostPort to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/f4ef1b2546d7
Change code that uses nsIURI.setHostAndPort() to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/6c437b464d61
Make NS_MutateURI methods warn immediately after calling method that fails r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/26dc46bd3ccc
Change code that sets nsIURI.pathQueryRef to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/ac8bd801fea3
Change code that sets nsIURI.filePath to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/b3dba84bd938
Change code that calls nsIURI.setQueryWithEncoding to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/4b418abe7e7a
Change code that sets nsIURI.userPass to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/b94f515b741c
Change URLWorker to use NS_MutateURI r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/0df1a74274d2
Change code that sets nsIURI.username to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/a9d894cb521e
Change code that sets nsIURI.password to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/b47076e8c5f8
Don't fail when setting an empty password if the username is already empty r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/d39e5d03e446
Change code that sets nsIURI.ref to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/2827f08f7b53
Change code that sets nsIURI.query to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/91477a3eabdf
Change code that sets nsIURI.port to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/d46afbe90f3c
Change code that sets nsIURI.host to use nsIURIMutator r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/2246691a04f8
If setting the host succeeds, SetHostPort should not return an error code r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/7ecadae9f269
Change code that sets nsIURI.scheme to use nsIURIMutator r=mayhemer
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/80a791b88011f7459d23a4f11088a88a4131995d
chore(mc): Port Bug 1433958 - Change code that sets nsIURI.ref to use nsIURIMutator r=mayhemer (#4012)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: