If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Cu.cloneInto cloneFunctions should create argument-cloning forwarders

VERIFIED FIXED in mozilla33

Status

()

Core
XPConnect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: Gijs)

Tracking

unspecified
mozilla33
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=1 s=33.1 [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

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?
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

3 years ago
Created attachment 8437020 [details] [diff] [review]
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...)
Attachment #8437020 - Flags: feedback?(bobbyholley)
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)?
Attachment #8437020 - Flags: feedback?(bobbyholley)
And yeah, just add something to the test_cloneInto.xul that makes sure that the object identities change for cloned functions.
(Assignee)

Comment 5

3 years ago
Created attachment 8437075 [details] [diff] [review]
make cloneInto create functions that clone their arguments,

D'oh. Well, this was certainly simpler. :-)
Attachment #8437075 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8437020 - Attachment is obsolete: true
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?
Attachment #8437075 - Flags: review?(bobbyholley) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8437092 [details] [diff] [review]
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
Attachment #8437092 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8437075 - Attachment is obsolete: true
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.
Attachment #8437092 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 9

3 years ago
(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...
(Assignee)

Comment 10

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

Carrying over r+ for this patch
Attachment #8437075 - Attachment description: make exportFunction create functions that clone their arguments, → make cloneInto create functions that clone their arguments,
Attachment #8437075 - Attachment is obsolete: false
Attachment #8437075 - Flags: review+
(Assignee)

Updated

3 years ago
Blocks: 1023120
(Assignee)

Comment 11

3 years ago
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
Attachment #8437075 - Flags: checkin+
(Assignee)

Comment 12

3 years ago
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.
Attachment #8437092 - Attachment is obsolete: true
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
(Assignee)

Comment 14

3 years ago
(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?
Flags: needinfo?(gbrown)
I'll see what I can sort out in bug 1023314.
Flags: needinfo?(gbrown)
(Assignee)

Comment 16

3 years ago
Relanded as remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00d5164a4049
https://hg.mozilla.org/mozilla-central/rev/00d5164a4049
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Comment 18

3 years ago
Marco, can you add this to this iteration?
Status: RESOLVED → VERIFIED
Flags: needinfo?(mmucci)
Flags: in-testsuite+
Whiteboard: p=1 s=33.1 [qa-]
Added to Iteration 33.1
Flags: needinfo?(mmucci)

Updated

3 years ago
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.