929783 - Fix an exact rooting hazard in InterAppComm::EnabledForScope

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Terrence Cole [:terrence]]

Attachment: hazard_InterAppComm-v0.diff

I switched this method to take a Handle instead of a raw object so that the object reference will be rooted across the call to Preferences::setBool. EnabledForScope appears to be referred to in various webidl files, although it is not defined in one. I'm not sure if this impacts anything in Codegen.py or not. It also seems to share its signature with Proxy::EnabledForScope; I'm not sure if this is a coincidence. Despite my uncertainty, try is green: https://tbpl.mozilla.org/?tree=Try&rev=79213f7b4c55

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 820685 [details] [diff] [review]
hazard_InterAppComm-v0.diff

Could you update 
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BFunc.3D.22funcname.22.5D


Looks like Codegen.py is fine
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py?rev=ceb4bd44eb34&mark=2064-2067,2081-2084,2100-2102,2114-2116#2055

Comment 2

[Andrew McCreight [:mccr8]]

> I'm not sure if this impacts anything in Codegen.py or not.

I'd guess that the bindings generation code just takes the string that it is given and pastes it somewhere, so it shouldn't CodeGen.py at all.  The surrounding code must be set up to deal with handles and so forth already or your patch would break it, presumably due to something bz fixed.  So, it is just individual places that need to be fixed.

As a lower priority followup, it would be a good idea to fix up all the Func=whatever methods so they take handles in their second argument, even though they are currently unused, so when people inevitably copy or modifying an existing one in a way that uses the obj argument, they won't get a rooting hazard.
  http://mxr.mozilla.org/mozilla-central/search?string=Func%3D&find=.webidl&findi=&filter=[Ff]unc%3D&hitlimit=&tree=mozilla-central

Comment 3

[Andrew McCreight [:mccr8]]

Though there are an awful lot of functions, so maybe leaving it alone is best. ;)

Comment 4

[Terrence Cole [:terrence]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Comment on attachment 820685 [details] [diff] [review]
> hazard_InterAppComm-v0.diff
> 
> Could you update 
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BFunc.3D.
> 22funcname.22.5D

Fixed.
 
> Looks like Codegen.py is fine
> http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.
> py?rev=ceb4bd44eb34&mark=2064-2067,2081-2084,2100-2102,2114-2116#2055

Great, Thanks Olli!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e2322cbc60

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Fixed.

This fix was wrong, as far as I can tell.

[Func] on an _interface_ passes through a handle.  But [Func] on an interface member passes in a raw JSObject*, and can't pass a handle so easily.

The documentation's example happens to be the latter case....

Comment 6

[Boris Zbarsky [:bzbarsky]]

I believe I have now fixed the documentation to match reality again, but please double-check!

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0e2322cbc60

Comment 8

[Terrence Cole [:terrence]]

(In reply to On vacation Oct 12 - Oct 27 from comment #6)
> I believe I have now fixed the documentation to match reality again, but
> please double-check!

Oh, wow, I totally missed that subtlety. Thanks for the catch, Boris!