572647 - XPCConvert::NativeInterface2JSObject should create COWs automatically

Status: RESOLVED FIXED

Description

[Olli Pettay [:smaug]]

https://bugzilla.mozilla.org/show_bug.cgi?id=572129#c2

Comment 1

[Johnny Stenback (:jst)]

mrbkap, can you have a look at his one? We're interested in this for branches ASAP.

Comment 2

[Robert Sayre]

blake, can we get an ETA on this?

Comment 3

[Blake Kaplan (:mrbkap) (leave, Aug. 16-Nov. 16)]

As a note, bug 523994 fixes this in a really clean way (and makes NativeInterface2JSObject actually legible too). Unfortunately, I'm bogged down in fixing-mochitest hell in that bug, so I'll fix this one separately so we can get it landed while I finish bug 523994.

Comment 4

[Blake Kaplan (:mrbkap) (leave, Aug. 16-Nov. 16)]

Created attachment 457749 [details] [diff] [review]
Proposed fix

Of special importance in reviewing this patch is the control flow in NI2JSO: I looked carefully and as far as I can tell, it is safe to have an early return here. Also, how we determine that we are returning to content is a little skeezy (using filename flags is pretty terrible). Also, figuring out that the object is chrome is not great. bug 523994 will again come to rescue us.

Comment 5

[Damon Sicore (:damons)]

Are we done here?  Can we land?

Comment 6

[Damon Sicore (:damons)]

Test?

Comment 7

[Robert Sayre]

blake?

Comment 8

[Blake Kaplan (:mrbkap) (leave, Aug. 16-Nov. 16)]

Sorry, the patch causes a ton of assertions on mochichrome tests. I'll attach a new one tomorrow.

Comment 9

[Damon Sicore (:damons)]

Where's that new patch, mrbkap?

Comment 10

[Damon Sicore (:damons)]

Blake, seriously, is there a new patch coming?

Comment 11

[Blake Kaplan (:mrbkap) (leave, Aug. 16-Nov. 16)]

(we'll need to do something special for the older branches)

Comment 12

[Damon Sicore (:damons)]

As soon as compartments lands, we should verify this is fixed.  Marcia?

Comment 13

[Marcia Knous [:marcia - needinfo? me]]

Blake: Is there any an easy way I can verify that this bug is fixed? I don't see a testcase readily available.

Comment 14

[Johnny Stenback (:jst)]

Marcia, the testcase in bug 572129 might show whether this got fixed by the compartments landing or not.

Comment 15

[Olli Pettay [:smaug]]

Marcia, did you manage to verify whether this is fixed or not?

Comment 16

[Marcia Knous [:marcia - needinfo? me]]

When I run the test case from the bug in Comment 14 using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101116 Firefox/4.0b8pre, I still get the 
alert dialog that shows "Components.stack."

Comment 17

[Johnny Stenback (:jst)]

So this bug as filed has been fixed, there's remaining bugs on making them default safe (bug 628410).

Marking FIXED.

Comment 18

[Al Billings [:abillings]]

Is the fact that bug 572129 is not fixed have any bearing on whether we want to consider this fixed? See https://bugzilla.mozilla.org/show_bug.cgi?id=572129#c23.

Comment 19

[Daniel Veditz [:dveditz]]

If we're going to fix this on 1.9.2 it needs to be now. If we're not ever going to do it we should just say that.

Comment 20

[Blake Kaplan (:mrbkap) (leave, Aug. 16-Nov. 16)]

I don't think we can really fix this on 1.9.2. COWs (as we were hoping to backport them when this bug was filed) have been replaced by new code that is heavily dependent on compartments.