892687 - Make returning object values not require fallible getters/methods

Status: NEW

(Core :: DOM: Bindings (WebIDL), defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now, returning an object from WebIDL bindings is always fallible, for several reasons:

1)  JS_NewObject can throw, either when allocating the wrapper or setting up its proto chain.

2)  JS_Define* can throw when setting up the proto chain.

3)  JS_WrapValue can throw.

I will claim that once bug 862848 is fixed, these can all only happen due to OOM.  Bobby, is that true for JS_WrapValue?

And at that point, I think we should just make these infallible on the binding side (with MOZ_CRASH as needed).

That would allow us to avoid outputting jitcode to check for exceptions and whatnot, which should make our jitcode faster and smaller and easier to understand.

Sequence and dictionary return values would remain fallible, since those might in fact need to allocate large chunks of memory depending on the length of the sequence and what's in the dictionary.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> is that true for JS_WrapValue?

I realize that JS_WrapValue on a string can do a big allocation; I'll keep that one fallible.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  Peter, how do you feel about MOZ_CRASH if someone tries to GetProtoObject() or GetConstructorObject() for a WebIDL binding on a global that does not have JSCLASS_DOM_GLOBAL?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #0)
> I will claim that once bug 862848 is fixed, these can all only happen due to
> OOM.  Bobby, is that true for JS_WrapValue?

JSCompartment::wrap only throws for OOM or if the XPConnect wrap hooks throw return null. This can happen if:
* JS_ObjectToOuterObject fails
* JS_GetPrototype or js::GetObjectProto fails
* JS_GetClassPrototype fails
* PreCreate throws on an XPCWN being wrapped cross-compartment
* WrapNativeToJSVal throws trying to create a new XPCWN reflector in a the target compartment.
* XPCNativeSet::GetNewOrUsed fails in the case of the above.
* Somebody tries to wrap a chrome-privileged eval or Function constructor into content.
* Wrapper::Renew (probably just OOM, but I didn't dig into it)
* OOM

The hairy ones shouldn't apply to things that we know are new-binding objects (unless the Function constructor is a new-binding object? anyway, I'm totally happy to MOZ_CRASH in that case). So I think we can treat JS_WrapValue as infallible here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We don't necessarily know these values are new-binding objects.  All we know is they're some JSObject.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> We don't necessarily know these values are new-binding objects.  All we know
> is they're some JSObject.

In what situations might we be returning an XPCWN here, especially an XPCWN with a spineless PreCreate hook?

Anyway, in general I'm happy to MOZ_CRASH here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> In what situations might we be returning an XPCWN here

Any time WebIDL has an external interface return value, say.  In the long-term, never.

> especially an XPCWN with a spineless PreCreate hook

The only XPCWN with PreCreate hooks are:  Window, nsDOMConstructor, Location, IDBEventTarget, History.  No idea whether they're spineless.  ;)

> Anyway, in general I'm happy to MOZ_CRASH here.

Excellent.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, I used to have patches for this, partly, but my computer decided to eat them.  :(

Given that, the lack of progress on the bug this depends on, and the fact that I think I've found a way to work around this in general, I'm just not going to worry about this.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Hmm.  Peter, how do you feel about MOZ_CRASH if someone tries to
> GetProtoObject() or GetConstructorObject() for a WebIDL binding on a global
> that does not have JSCLASS_DOM_GLOBAL?

Fine by me.

Comment 9

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.