don't depend on Function.prototype.toSource

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

unspecified
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 642196 [details] [diff] [review]
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.
Attachment #642196 - Flags: review?(gavin.sharp)
What a coincidence! I think Mark was working on doing something like this already.
Attachment #642196 - Flags: review?(gavin.sharp) → review?(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 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...
Attachment #642196 - Flags: review?(mhammond) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 642631 [details] [diff] [review]
don't use toSource

Renamed files and use loadSubScript.
Attachment #642196 - Attachment is obsolete: true
Attachment #642631 - Flags: review?(mhammond)
(Assignee)

Comment 5

5 years ago
Created attachment 642642 [details] [diff] [review]
don't use toSource

Rebased.
Attachment #642631 - Attachment is obsolete: true
Attachment #642631 - Flags: review?(mhammond)
Attachment #642642 - Flags: review?(mhammond)
Blocks: 770679
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.
Attachment #642642 - Flags: review?(mhammond) → review-
(Assignee)

Comment 7

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

Comment 9

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

Comment 10

5 years ago
Created attachment 642803 [details] [diff] [review]
don't use toSource

Killed the NetUtil import.
Assignee: nobody → bpeterson
Attachment #642642 - Attachment is obsolete: true
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 :)
Attachment #642803 - Flags: review+
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?
Attachment #642803 - Flags: feedback-
(Assignee)

Comment 13

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

Comment 14

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(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)
(I commented in bug 761723, we can discuss further there)
(Assignee)

Comment 17

5 years ago
At least the non-test changes can be landed.
(Assignee)

Comment 18

5 years ago
Created attachment 643171 [details] [diff] [review]
don't use toSource

Just component changes.
Attachment #642803 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa560d1645d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6aa560d1645d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
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
https://hg.mozilla.org/mozilla-central/rev/6f5bf0ee99ce
You need to log in before you can comment on or make changes to this bug.