Several issues here, worth a careful review.

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 |;|

> 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.