788405 - importScripts failure

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

When I was working on the sample provider I tried moving several functions into an imported script.  Apparently, those functions were not available to the script importing them, I had to put everything in the main worker script for everything to work.

demo provider: https://github.com/mixedpuppy/socialapi-demo/

Comment 1

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

Attachment: Use evalInSandbox to evaluate the script code

This surprises me, but the root of the problem is that:

---8<----
function test() {
  eval("function f() {return 'hi';}; x='hi';");
}
test();
x;
f();
---8<---

defines a global 'x' but does *not* define a global 'f'.

This patch exposes '_evalInSandbox' to the worker code and has importScripts use it, plus tests.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 659063 [details] [diff] [review]
Use evalInSandbox to evaluate the script code

I don't like exposing that, but it is the only way I see this working until real workers are used.

Comment 3

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

I came across this project, which is kind of interesting because its goal is to do something similar to what we're doing (re-implement a "Worker"):
http://code.google.com/p/fakeworker-js/source/browse/src/javascript/fakeworker.js#293

They seem to be trying a bunch of hacky tricks to get a working importScripts(), but they're all commented out so I guess they didn't find one that worked.

Comment 4

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

Comment on attachment 659063 [details] [diff] [review]
Use evalInSandbox to evaluate the script code

If this doesn't work, then I assume no providers are relying on it. Maybe it's better to have a missing importScripts implementation than to have a hacky one like this? If/when we start using real SharedWorkers, this problem would be solved.

I don't feel strongly. If you think we need this now for completeness I guess we can land this.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

FWIW, importScripts isn't entirely necessary. Providers can use the sidebar DOM to do all of the communication with the server if they want (since we no longer destroy the sidebar now).

Comment 6

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

(In reply to Jared Wein [:jaws] from comment #5)
> FWIW, importScripts isn't entirely necessary. Providers can use the sidebar
> DOM to do all of the communication with the server if they want (since we no
> longer destroy the sidebar now).

Following that through, the worker itself isn't strictly necessary either (except maybe a trivial "echo" worker to facilitate toast and chats).  However, I don't think we should be encouraging people to avoid leveraging workers or we will lose any potential benefits of moving to a real worker, so I intend landing this change.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e96444da92

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f496fceb64ad for xpcshell test_SocialService.js bustage.

Comment 9

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

This patch wasn't involved in the bustage, so re-pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c948a6448ef

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c948a6448ef

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/32bd98045465