Closed Bug 865975 Opened 7 years ago Closed 7 years ago

Better rooting for WebIDL callback invocations

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

Several issues here, worth a careful review.
Attachment #742188 - Attachment is obsolete: true
Attachment #742188 - Flags: review?(bobbyholley+bmo)
Comment on attachment 742190 [details] [diff] [review]
Better rooting for the 'this', 'callable', and 'rval' values in WebIDL callbacks.

Review of attachment 742190 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with comments addressed.

::: dom/bindings/Codegen.py
@@ +8705,5 @@
>                       "}\n")
>  
>          bodyWithThis = string.Template(
>              setupCall+
> +            "JS::Rooted<JSObject*> thisObjJS =\n"

Wait, what's the deal with this? You need a cx to create a RootedObject, and the constructor is explicit. How does this compile?

@@ +8855,5 @@
>  
>      def getResultConversion(self):
>          replacements = {
>              "val": "rval",
> +            "valPtr": "rval.address()",

Does this become a hazard? Do we need to pass it as a MutableHandleValue? I just don't know what this code does - it's probably fine.

@@ +9084,5 @@
>                                  descriptor,
>                                  needThisHandling=False)
>  
>      def getRvalDecl(self):
> +        return "JS::Rooted<JS::Value> rval(cx, JS::UndefinedValue());;\n"

Trailing |;|
Attachment #742190 - Flags: review?(bobbyholley+bmo) → review+
> How does this compile?

Ouch.  It compiles because it's in a template that's never instantiated in dom/bindings and I didn't do a full-tree compile.  content/events in fact does not compile with this patch.  ;)  Fixed:

            "JS::Rooted<JSObject*> thisObjJS(s.GetContext(),\n"
            "  WrapCallThisObject(s.GetContext(), CallbackPreserveColor(), thisObj));\n"


> Does this become a hazard?

I don't think so; this is passing in a Value* and if the value changes due to GC the callee will therefore see the new value when it dereferences the pointer.

Long-term we do in fact want to move to a MutableHandle here, but that involves fixing all callees to take handles.

> Trailing |;|

Fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85886f84801
Flags: in-testsuite-
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/d85886f84801
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.