Make ErrorResult pass the rooting analysis

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: evilpie, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 years ago
The rooting analysis goes crazy because of the unioned "JS::Value mJSException". But we also probably can't accept just always rooting it.
Blocks: 580070
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?
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?
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.
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.  ;)
Summary: Figure out what to do about ErrorResult → Make ErrorResult pass the rooting analysis
(Reporter)

Comment 5

5 years ago
This was fixed, otherwise we wouldn't have 3 hazards right now.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.