Last Comment Bug 547712 - Remove scatter from js shell
: Remove scatter from js shell
Status: RESOLVED DUPLICATE of bug 650411
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
: 624099 (view as bug list)
Depends on: 545962
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-22 06:08 PST by Jason Orendorff [:jorendorff]
Modified: 2011-08-12 00:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (52.48 KB, patch)
2010-02-22 18:10 PST, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Splinter Review
Disable remaining scatter-based test. (624 bytes, patch)
2011-01-10 12:39 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2010-02-22 06:08:43 PST
It has been pretty broken since shavarrays. Shell workers (bug 545962) are the new plan.
Comment 1 Jason Orendorff [:jorendorff] 2010-02-22 08:17:17 PST
Several of the tests are worth carrying forward from scatter() to shell workers. I've done the translations, but those that need gc() or some other shell function don't work yet, because workers get a crippled global environment. So I'll have to fix that first.
Comment 2 Jason Orendorff [:jorendorff] 2010-02-22 18:10:38 PST
Created attachment 428347 [details] [diff] [review]
v1
Comment 3 Igor Bukanov 2010-02-22 23:04:23 PST
(In reply to comment #0)
> It has been pretty broken since shavarrays. 

It was unsafe since e4x appearance in SM. But that does not mean that a safe use does not exist. To make scatter safe one just need need to avoid unsafe features. For this reason most if not all scatter tests are safe. Moreover, the scatter is invaluable currently for the test coverage of bugs related to the GC and to models WebWorkers in the absence of shell workers.

Thus we should consider removal of scatter only when we would get workers working in the shell.
Comment 4 Jason Orendorff [:jorendorff] 2010-02-23 07:46:39 PST
Right. Only two of the scatter tests are really broken. dekker.js never terminates under the JIT; for-in.js crashes because it shares arrays across threads.

This patch is intended to go in after the patch in bug 545962. It replaces the scatter tests with Worker tests where possible.

A few of the scatter tests actually test contention on objects shared across threads. The patch just deletes those.
Comment 5 Jason Orendorff [:jorendorff] 2010-02-23 08:14:06 PST
The reason I see scatter as "broken" is that I don't really understand the thread safety bugs we have well enough to tell the safe uses from unsafe ones.

For example, I said for-in.js crashes because it shares arrays. I really don't know, though.  The dangers with arrays, I think, are that resizing can race with dslots accesses, and slowification can race with dense array operations. for-in.js doesn't resize or slowify as far as I can see, but it does sporadically crash in array_enumerate.
Comment 6 Igor Bukanov 2010-03-01 12:24:06 PST
I am still reviewing the patch. My current worry is that using the Worker.scripts hack would not allow for the tests to run in a browser. On the other hand splitting the tests in many small files is very inconvenient. 

AFAICS there are two workarounds. First we can try to support the data url for workers. This way one can use new Worker("data:,"+encodeURLPart(worker_source)) to run inlined worker_source. But a quick test shows that FF does not support data url for workers...

If that is indeed true, then we can create a single eval worker source like:

  onmessage = function(e) { eval(e.data); }

That should cover most test cases.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2010-03-01 16:06:36 PST
Nothing in the workers spec suggests that data: URLs wouldn't be valid workers.  Bug, or misdiagnosis?
Comment 8 Igor Bukanov 2010-03-01 17:45:25 PST
For a test like:

<html>
<script>
function f() {
   var w = new Worker("data:,"+encodeURIComponent("1;"));
   w.onmessage = function(arg) { alert(arg); }
}

</script>
<body onload="f()">
</body>
</html>

I am getting in the error console:

Error: Failed to load script: data:,1%3B (nsresult = 0x805303f4)
Comment 9 Jesse Ruderman 2010-07-23 07:19:58 PDT
fwiw, all the tests using scatter() were disabled in bug 572600.
Comment 10 Jim Blandy :jimb 2011-01-10 12:09:07 PST
*** Bug 624099 has been marked as a duplicate of this bug. ***
Comment 11 Jim Blandy :jimb 2011-01-10 12:39:58 PST
Created attachment 502575 [details] [diff] [review]
Disable remaining scatter-based test.

This is less drastic, and avoids intermittent failures until we are ready to clean house.
Comment 12 Jim Blandy :jimb 2011-01-10 14:31:36 PST
http://hg.mozilla.org/tracemonkey/rev/e9a3c1ba86ea
[bug should remain open]
Comment 13 Luke Wagner [:luke] 2011-08-12 00:50:30 PDT

*** This bug has been marked as a duplicate of bug 650411 ***

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