773934 - don't depend on Function.prototype.toSource

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, enhancement)

Description

[:Benjamin Peterson]

Attachment: don't use Function.prototype.toSource

Bug 761723 will cause chrome code to not support Function.prototype.toSource(). Therefore, social API needs to stop using it. I think this allows a nicer separation of privileged/unprivileged code.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

What a coincidence! I think Mark was working on doing something like this already.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 642196 [details] [diff] [review]
don't use Function.prototype.toSource

A nit, but I'm not that fond of the new filenames - eg FrameWorker-base.jsm is really only holding the base for the "port" implementation, so "port" should probably be in the name.  Similarly for FrameWorker-child - the "child" portion isn't very descriptive (there are ultimately 2 objects that extend the base).  The DOM objects these are modelling are called a "MessagePort", so I'd think MessagePortBase.jsm and MessagePortWorker.jsm are suitable names?

Also, in https://bugzilla.mozilla.org/show_bug.cgi?id=770679#c2 it is recommended that the code  use Services.scriptloader.loadSubScript(jsURI, sandbox) rather than the nested asyncFetch calls in the patch.  I'll ask ochameau for feedback on that part.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 642196 [details] [diff] [review]
don't use Function.prototype.toSource

On reflection, I think the loadSubScript approach would be better - this patch makes just the fetching of the script text async, but the evalInSandbox() itself is still sync.  Given loadSubScript is able to cache the compiled versions of the scripts, I'd expect that in general the loadSubScript version would block the main thread for less time than the NetUtil.asyncFetch/evalInSandbox approach.  If you disagree then we should drag in ochameau for his thoughts...

Comment 4

[:Benjamin Peterson]

Attachment: don't use toSource

Renamed files and use loadSubScript.

Comment 5

[:Benjamin Peterson]

Attachment: don't use toSource

Rebased.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 642642 [details] [diff] [review]
don't use toSource

I think the line:

+Cu.import("resource://gre/modules/NetUtil.jsm");

is not needed.  Apart from that, I think the change to the runtime code is good and gets r+.  However, I think that the changes to the tests aren't necessary (as they don't use prototype.toSource(), just functionob.toSource()) and the changes actually detract from the test code as the logic for each test gets split in ways that make the tests harder to read and comprehend.  So I'm going to r- this patch, but you can assume an r+ from me for the parts of the patch which only touch the runtime.

Comment 7

[:Benjamin Peterson]

(In reply to Mark Hammond (:markh) from comment #6)
> Comment on attachment 642642 [details] [diff] [review]
> don't use toSource
> 
> I think the line:
> 
> +Cu.import("resource://gre/modules/NetUtil.jsm");

I'll kill it.

> 
> is not needed.  Apart from that, I think the change to the runtime code is
> good and gets r+.  However, I think that the changes to the tests aren't
> necessary (as they don't use prototype.toSource(), just
> functionob.toSource()) and the changes actually detract from the test code
> as the logic for each test gets split in ways that make the tests harder to
> read and comprehend.  So I'm going to r- this patch, but you can assume an
> r+ from me for the parts of the patch which only touch the runtime.

[some function object].toSource is the same thing as Function.prototype.toSource by prototype inheritance.

I can put the functions back in the test file, but they would have to be in strings, which is not particularly nice. browser_framework.js was using separate worker_.js files before this patch, so it's not a new technique in that file.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Benjamin Peterson from comment #7)

> I can put the functions back in the test file, but they would have to be in
> strings, which is not particularly nice.

Agree that strings would suck - but I don't understand why that becomes necessary - is there something that says functionob.toSource() is going to stop working?  If so, I agree we need to split as you did.

> browser_framework.js was using
> separate worker_.js files before this patch, so it's not a new technique in
> that file.

Only tests which *need* to be split currently are, and they are only tests that need to live on a specific origin.

Comment 9

[:Benjamin Peterson]

(In reply to Mark Hammond (:markh) from comment #8)
> (In reply to Benjamin Peterson from comment #7)
> 
> > I can put the functions back in the test file, but they would have to be in
> > strings, which is not particularly nice.
> 
> Agree that strings would suck - but I don't understand why that becomes
> necessary - is there something that says functionob.toSource() is going to
> stop working?  If so, I agree we need to split as you did.

Bug 761723 will cause all functions created in a chrome to return the same value as native functions. Namely

function f() {
   [native function]
}

Since browser_framework is a chrome test, this restriction applies.

Comment 10

[:Benjamin Peterson]

Attachment: don't use toSource

Killed the NetUtil import.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 642803 [details] [diff] [review]
don't use toSource

damn - it seems a real shame functionob.toSource() in chrome, for functions also created in chrome, to break like this is a real shame - but this isn't the place to complain about it :)

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 642803 [details] [diff] [review]
don't use toSource

This seems like a fine place to complain. :) I don't think breaking this is feasible! Doesn't it break all sorts of things beyond just this?

Comment 13

[:Benjamin Peterson]

(In reply to Mark Hammond (:markh) from comment #11)
> Comment on attachment 642803 [details] [diff] [review]
> don't use toSource
> 
> damn - it seems a real shame functionob.toSource() in chrome, for functions
> also created in chrome, to break like this is a real shame - but this isn't
> the place to complain about it :)

Yeah. FWIW, very few chrome tests were relying on toSource() to begin with. Of those, this was the only case, where using toSource() was a nicer solution. It's possible it could be enabled just for test files in the future.

Comment 14

[:Benjamin Peterson]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #12)
> Comment on attachment 642803 [details] [diff] [review]
> don't use toSource
> 
> This seems like a fine place to complain. :) I don't think breaking this is
> feasible! Doesn't it break all sorts of things beyond just this?

No, in fact, browser which passes all tests without chrome toSource can be built with this patch and another 15kb one.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Benjamin Peterson from comment #14)
> No, in fact, browser which passes all tests without chrome toSource can be
> built with this patch and another 15kb one.

I don't think our test suite is sufficient for determining the compatibility impact of this change. Many add-ons use function.toSource for monkey-patching:

https://mxr.mozilla.org/addons/search?string=.toSource%28%29.replace%28&find=\.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons

(use your LDAP password)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(I commented in bug 761723, we can discuss further there)

Comment 17

[:Benjamin Peterson]

At least the non-test changes can be landed.

Comment 18

[:Benjamin Peterson]

Attachment: don't use toSource

Just component changes.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

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

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6aa560d1645d

Comment 21

[:Gavin Sharp [email: [e-mail]]]

I pushed a followup to remove now-unused function and add license headers to the new files:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5bf0ee99ce

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f5bf0ee99ce