Open Bug 892687 Opened 11 years ago Updated 2 years ago

Make returning object values not require fallible getters/methods

Categories

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

defect

Tracking

()

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

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.
> 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.
OS: Mac OS X → All
Hardware: x86 → All
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?
OS: All → Mac OS X
Hardware: All → x86
Flags: needinfo?(peterv)
Flags: needinfo?(bobbyholley+bmo)
OS: Mac OS X → All
Hardware: x86 → All
(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.
Flags: needinfo?(bobbyholley+bmo)
We don't necessarily know these values are new-binding objects. All we know is they're some JSObject.
Flags: needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(bobbyholley+bmo)
> 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.
Depends on: 864228
Depends on: 862848
Depends on: 905168
No longer blocks: 924682
Blocks: 916851
Flags: needinfo?(peterv)
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.
Assignee: bzbarsky → nobody
No longer blocks: 916851
(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.
Flags: needinfo?(peterv)
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.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
Component: DOM: Core & HTML → DOM: Bindings (WebIDL)
Priority: P5 → --
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.