Make nsIURI::SchemeIs infallible

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
Last month

People

(Reporter: qdot, Assigned: qdot)

Tracking

(Blocks 2 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment)

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

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.
Whiteboard: [necko-triaged]
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85cbb065250d
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:

  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.

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(kyle)
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Depends on: 1519711

(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)
You need to log in before you can comment on or make changes to this bug.