Closed Bug 1518956 Opened 2 years ago Closed 2 years ago

Make nsIURI::SchemeIs infallible


(Core :: Networking, enhancement, P3)




Tracking Status
firefox66 --- fixed


(Reporter: qdot, Assigned: qdot)


(Blocks 1 open bug)


(Whiteboard: [necko-triaged])


(1 file)

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.

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.

Priority: -- → P3

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. 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.

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
Whiteboard: [necko-triaged]
Pushed by
Make C++ infallible/simplified versions of nsIURI::SchemeIs; r=valentin

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:"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.

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(kyle)
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

(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.

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(kyle)

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

You need to log in before you can comment on or make changes to this bug.