1022002 - Cu.cloneInto cloneFunctions should create argument-cloning forwarders

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

When baku implemented cloneFunctions for Cu.cloneInto, we decided to to use non-cloning forwarders (a la Cu.makeObjectPropsNormal), rather than the safer cloning function forwarders (a la Cu.exportFunction).

Gijs and I were just talking, and we think we should be doing the safe thing. There aren't any references to Cu.cloneInto or ObjectWrapper.jsm in addons, and there are only two extraneous uses of Object.wrapper.jsm in the tree.

baku, was there any other reason we did non-cloning forwarders?

Comment 1

[Andrea Marchesini [:baku]]

> baku, was there any other reason we did non-cloning forwarders?

No. We just tried to reproduce the same behaviour of the previous CloneInto method in ObjectWrapper.jsm.

Comment 2

[:Gijs (he/him)]

Attachment: make cloneInto use function forwarders when using the cloneFunctions option,

This is what I came up with... does this look OK, and how could I create a test for this (xpcshell, I guess? H, but not entirely sure how best to test that this works as advertised...)

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8437020 [details] [diff] [review]
make cloneInto use function forwarders when using the cloneFunctions option,

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3535,5 @@
>            return nullptr;
>  
> +      RootedValue val(cx, ObjectValue(*obj));
> +      RootedValue options(cx, UndefinedValue());
> +      if (!xpc::ExportFunction(cx, val, data->mScope, options, &functionValue))

I don't know if we really want to be calling the full ExportFunction here. Don't we just want to pass doClone = true to xpc::NewFunctionForwarder (and then remove the bool param altogether)?

Comment 4

[Bobby Holley (:bholley)]

And yeah, just add something to the test_cloneInto.xul that makes sure that the object identities change for cloned functions.

Comment 5

[:Gijs (he/him)]

Attachment: make cloneInto create functions that clone their arguments,

D'oh. Well, this was certainly simpler. :-)

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8437075 [details] [diff] [review]
make cloneInto create functions that clone their arguments,

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

Looks good, but can you remove the bool param from NewFunctionForwarder as well?

Comment 7

[:Gijs (he/him)]

Attachment: make cloneInto and exportFunction create functions that clone their arguments,

Yes. But this is more of a scarypants change, so: https://tbpl.mozilla.org/?tree=Try&rev=8f7dd31439ca

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8437092 [details] [diff] [review]
make cloneInto and exportFunction create functions that clone their arguments,

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

Oh dang, I'd forgotten that Cu.makeObjectPropsNormal was still using the doClone = false variant. r=me if this is green or can easily be made green, otherwise, feel free to land the other patch.

Comment 9

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #8)
> Comment on attachment 8437092 [details] [diff] [review]
> make cloneInto and exportFunction create functions that clone their
> arguments,
> 
> Review of attachment 8437092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh dang, I'd forgotten that Cu.makeObjectPropsNormal was still using the
> doClone = false variant. r=me if this is green or can easily be made green,
> otherwise, feel free to land the other patch.

Looks like the social conversion to webidl and friends will need to happen before this works:

12:17:39     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindow.js | Console message: [JavaScript Error: "FrameWorker: Port MessagePort(portType='parent', portId=49) handler failed: Error: CloneNonReflectorsWrite error

followed by lots of social mochitest-browser sadness. Otherwise, this patch seems to have done fine...

Not 100% sure what this error means, so equally unsure of how to fix it, so I suppose I'll be landing the earlier patch tomorrow...

Comment 10

[:Gijs (he/him)]

Comment on attachment 8437075 [details] [diff] [review]
make cloneInto create functions that clone their arguments,

Carrying over r+ for this patch

Comment 11

[:Gijs (he/him)]

Comment on attachment 8437075 [details] [diff] [review]
make cloneInto create functions that clone their arguments,

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d172e86ce2e

Comment 12

[:Gijs (he/him)]

Comment on attachment 8437092 [details] [diff] [review]
make cloneInto and exportFunction create functions that clone their arguments,

Moved this patch to bug 1023120, so obsoleting here.

Comment 13

[Ed Morley [:emorley]]

This unfortunately appears to increase runtimes on Android 2.3 reftest-6, which means it now goes over the max job time limit (which it was pretty close to before, to be fair) - limit is 60 mins, it used to take 57-58 mins, but with this patch now takes 58-66 mins:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=b1d4ab072fc2&tochange=d1cc6c047c6c&jobname=Android%202.3.*plain-reftest-6

If this increase was expected, could you coordinate with gbrown about either increasing the max runtime for Android 2.3 reftests, or increasing the number of chunks? If it wasn't expected, could we try to reduce the impact here (slowly creeping up test runtimes is something we've tried to prevent before).

Since this is causing quite a lot of orange on Android 2.3 reftest-6 I'm going to have to back this out for now, either way:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b8d8f1d448

Comment 14

[:Gijs (he/him)]

(In reply to Ed Morley [:edmorley UTC+0] from comment #13)
> This unfortunately appears to increase runtimes on Android 2.3 reftest-6,
> which means it now goes over the max job time limit (which it was pretty
> close to before, to be fair) - limit is 60 mins, it used to take 57-58 mins,
> but with this patch now takes 58-66 mins:
> https://tbpl.mozilla.org/?tree=Mozilla-
> Inbound&fromchange=b1d4ab072fc2&tochange=d1cc6c047c6c&jobname=Android%202.3.
> *plain-reftest-6
> 
> If this increase was expected, could you coordinate with gbrown about either
> increasing the max runtime for Android 2.3 reftests, or increasing the
> number of chunks? If it wasn't expected, could we try to reduce the impact
> here (slowly creeping up test runtimes is something we've tried to prevent
> before).
> 
> Since this is causing quite a lot of orange on Android 2.3 reftest-6 I'm
> going to have to back this out for now, either way:
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b8d8f1d448

So, while initial retriggers certainly made it look like this patch's fault, logically, I was skeptical. The code I changed (cloneInto passing 'cloneFunctions: true', ie http://mxr.mozilla.org/mozilla-central/search?string=cloneFunctions&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central, where nobody actually uses the ObjectWrapper.wrap instance) has no in-tree consumers except for some xpcshell tests that check that it works as advertised.

With the benefit of some more time and 20/20 hindsight, it seems like it really wasn't this patch's fault:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=b1d4ab072fc2&tochange=8a0e868c47c2&jobname=Android%202.3.*plain-reftest-6

In particular, looking at 85ddc0b5770e until 46f584afbed5, they all hover around 57/58 minutes again.



Seeing as we're now having innocent victims of what I presume are infra-related slowdowns, can we up the max runtime and/or chunk more? gbrown?

Comment 15

[Geoff Brown [:gbrown]]

I'll see what I can sort out in bug 1023314.

Comment 16

[:Gijs (he/him)]

Relanded as remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00d5164a4049

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/00d5164a4049

Comment 18

[:Gijs (he/him)]

Marco, can you add this to this iteration?

Comment 19

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1