The default bug view has changed. See this FAQ.

importScripts failure

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: markh)

Tracking

unspecified
Firefox 18
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 fixed)

Details

(Whiteboard: [Fx17][qa-])

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

5 years ago
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.
Assignee: nobody → mhammond
Attachment #659063 - Flags: review?(gavin.sharp)
Attachment #659063 - Flags: feedback?(mixedpuppy)
(Reporter)

Comment 2

5 years ago
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.
Attachment #659063 - Flags: feedback?(mixedpuppy) → feedback+
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 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.
Attachment #659063 - Flags: review?(gavin.sharp) → review+
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).
(Assignee)

Comment 6

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

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e96444da92
Status: NEW → ASSIGNED
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f496fceb64ad for xpcshell test_SocialService.js bustage.
(Assignee)

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c948a6448ef
https://hg.mozilla.org/mozilla-central/rev/9c948a6448ef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #659063 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/32bd98045465
status-firefox17: --- → fixed
Whiteboard: [Fx17] → [Fx17][qa-]
You need to log in before you can comment on or make changes to this bug.