Last Comment Bug 788405 - importScripts failure
: importScripts failure
Status: RESOLVED FIXED
[Fx17][qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Mark Hammond [:markh]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-04 23:02 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-10-16 16:28 PDT (History)
5 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Use evalInSandbox to evaluate the script code (2.72 KB, patch)
2012-09-06 17:40 PDT, Mark Hammond [:markh]
gavin.sharp: review+
mixedpuppy: feedback+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-09-04 23:02:57 PDT
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] 2012-09-06 17:40:28 PDT
Created attachment 659063 [details] [diff] [review]
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) 2012-09-07 12:05:42 PDT
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: gavin@gavinsharp.com] 2012-09-10 06:46:50 PDT
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: gavin@gavinsharp.com] 2012-09-10 06:51:20 PDT
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) 2012-09-10 06:56:03 PDT
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] 2012-09-10 16:47:28 PDT
(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 8 Phil Ringnalda (:philor, back in August) 2012-09-11 21:01:36 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f496fceb64ad for xpcshell test_SocialService.js bustage.
Comment 9 Mark Hammond [:markh] 2012-09-11 21:52:25 PDT
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] 2012-09-12 13:54:09 PDT
https://hg.mozilla.org/mozilla-central/rev/9c948a6448ef
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-25 11:40:53 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/32bd98045465

Note You need to log in before you can comment on or make changes to this bug.