1014296 - specialpowersAPI.js entrains potential garbage

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Due two two missing variable declarations, the special powers object risks entraining potential garbage on the message manager global object.

It took khuey and me 3 hours to track this down (running a specific set of mochitests with a patch that changed the tests to use SpecialPowers wrappers).

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

And for posterity, here's the bit of log that pointed the problem out to us:

via XPCWrappedNative::mFlatJSObject :
0x7f72808e6f60 [ContentFrameMessageManager 7f7279544520]
    --[args]--> 0x7f72725964c0 [Array <no private>]
    --[objectElements[0]]--> 0x7f7272825980 [Proxy <no private>]
    --[private]--> 0x7f727259b780 [Object <no private>]
    --[wrappedObject]--> 0x7f72728282b0 [XPCWrappedNative_NoHelper 7f727dc202c0]

where that wrapped native was expected to have died already.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

I had to add an additional couple of var declarations that don't have an effect due to my "use strict", but I think it's worth it.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8426662 [details] [diff] [review]
Fix

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

To make sure I understand correctly - it's only ever 1 piece of garbage at a time, since we overwrite the old value each time, right?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Yeah. I probably wouldn't have noticed this if I'd run the whole suite of tests (e.g. on tinderbox), but only testing my patches that ended up passing a non-CC'd object through a specialpowers object wrapper leaked that content window.

Comment 5

[Nicholas Nethercote [inactive]]

Do we have a bug on using "use strict;" everywhere?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fortunately, I sent this patch to try – it fails spectacularly due to another strict mode violation: SpecialPowers.prototype.isDebugBuild tries to 'delete' itself as a 'get once getter'; however, it tries to delete itself off of 'this', which is not the prototype. I'm about to attach a new patch.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix v2

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b2561ccb9e7

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Do we have a bug on using "use strict;" everywhere?

I think we've converted most, if not all of the code. I don't know if we have protections in place to prevent people from adding non-strict-mode JSMs, though.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b2561ccb9e7