854626 - Make ErrorResult pass the rooting analysis

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tom S [:evilpie]]

The rooting analysis goes crazy because of the unioned "JS::Value mJSException". But we also probably can't accept just always rooting it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Indeed, having anything in mJSException is a rare case and the cost of rooting/unrooting this random slot no one will ever write to normally is not acceptable.

If we ignore the rooting analysis for the moment, is the current behavior correct?  I assume it's not, with a moving GC, right?  What would correct behavior be?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So for example, I could almost certainly just store a "double" and reinterpret_cast it to JS::Value as needed.  But that would obfuscate the code and not really help things be any more rooted, right?

Comment 3

[Andrew McCreight [:mccr8]]

If these are never copied around, we could manually root the object using one of ye olde rooting APIs if it gets an exception put into it, and unroot in the destructor.  One of the old rooting APIs even does some refcounting, though maybe that's going away.

Comment 4

[Boris Zbarsky [:bzbarsky]]

ErrorResult objects are never copied around.  Note the private-and-deleted copy constructor and lack of relevant operator=.

Setting the mJSException (via ThrowJSException) already calls JS_AddNamedValueRoot on &mJSException.

ReportJSException calls JS_RemoveValueRoot.

The destructor asserts that WouldReportJSException has been called; the contract is that any code that plans to call ReportJSException must also call WouldReportJSException (even if there is no exception, which is important!).

I _did_ try to make this stuf sane.  ;)

Comment 5

[Tom S [:evilpie]]

This was fixed, otherwise we wouldn't have 3 hazards right now.