1518956 - Make nsIURI::SchemeIs infallible

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

nsIURI::SchemeIs will only fail on null arguments now. It used to have more error cases, but most of these have been removed. Make SchemeIs infallible to clean up the C++ URI.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

This blocks bug 1469429 because I ran into this trying to do some DocShell cleanup.

This should remove the nsContentUtils SchemeIs call that basically does an infallible return, too.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

There is a massive amount of old-style SchemeIs usage around the codebase, and some of it is rather gross due to awkward compound logic (i.e. https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/chrome/nsChromeRegistryChrome.cpp#654). Since all these sites required outside variable storage due to XPCOM, it's not simple to do a mass find/replace to clean it up. I'm just landing the IDL definitions in this bug, then will break out cleanup per top/second level directories into other bugs. The current code won't break due to this, so we can do cleanup in stages.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1518956 - Make C++ infallible/simplified versions of nsIURI::SchemeIs;

SchemeIs only throws exceptions on null arguments now. Assert
arguments, as they should never be null anyways, and create an
infallible C++ version. Also, create simplified versions for most
protocols used around the code base, to reduce raw string passing in
code.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85cbb065250d
Make C++ infallible/simplified versions of nsIURI::SchemeIs; r=valentin

Comment 5

[Boris Zbarsky [:bzbarsky]]

Unfortunately, I don't think the asserts that landed in this bug are OK. The idl says:

  boolean schemeIs(in string scheme);

and xpconnect will happily allow JS to pass null for a "string" argument, making it into a nullptr char* on the C++ side. So there's nothing preventing null ending up in there, from the JS side.

A testcase that crashes with these changes is evaluating this in the browser console:

  Services.io.newURI("data:text/plain,aaa").schemeIs(null)

It might be OK to just have schemeIs return false when the string arg is null, though, instead of asserting that it's not null.

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/85cbb065250d

Comment 7

[Valentin Gosu [:valentin] (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)

It might be OK to just have schemeIs return false when the string arg is null, though, instead of asserting that it's not null.

Great point. I put up a patch in bug 1519711.

Comment 8

[Masatoshi Kimura [:emk]]

Maybe [infallible] annotation should be used instead of writing an infallible variant manually.