1045756 - add test to validate interfaces exposed on worker global

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently when we implement a new interface we add it to:

  dom/tests/mochitest/general/test_interfaces.html

In order to verify that only correct interfaces are exposed to the web.  We don't currently check what is exposed on the worker global.

It would be nice to have a test for workers as well.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Test DOM interfaces exposed on worker global.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8464146 [details] [diff] [review]
Test DOM interfaces exposed on worker global.

>+++ b/dom/workers/test/test_worker_interfaces.js
>+    // NB: We haven't bothered to resolve constants like Infinity and NaN on
>+    // Xrayed windows (which are seen from the XBL scope). We could support
>+    // this if needed with some refactoring.
>+    {name: "Infinity", xbl: false},
...
>+    {name: "NaN", xbl: false},

There is no XBL in workers.  Just make Infinity and NaN unconditional and remove that comment.

>+++ b/dom/workers/test/worker_driver.js

Might be nice to have a comment about how this and worker_wrapper allow you to do ok() and is() in worker tests and handle it for you.

>+++ b/dom/workers/test/worker_wrapper.js
>+  dump("OK: " + !!a + "  =>  " + a + " " + msg + "\n");

':' after the |a +| bit, like in the message you post?

>+  dump("IS: " + (a===b) + "  =>  " + a + " | " + b + " " + msg + "\n");

Similar.

That's assuming we want to keep the dump() statements, which seems reasonable.

>+var WorkerTest = {

Why do we need both the WorkerTest.done() variant and the workerTestDone variant?  Seems like a lot of code duplication for no obvious reason.

>+  getPrefs: function(prefs, cb) {

So this can be a bit of a footgun.  Say the worker script being tested does:

  getPrefs(["a"], function() { /* stuff */ });
  getPrefs(["b"], function() { /* other stuff });

Then the "other stuff" will end up with the pref value for "a".

Does it make sense to have some sort of sequence id for these requests that we capture in the event listener closure and send back and forth in the messages?

Alternately, we could set a global flag in the worker if we have a listener pending and just fail the test if the worker script attempts to add another listener while one is pending.  Might be simpler and still catch people screwing up.

r=me with those issues addressed

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #2)
> >+var WorkerTest = {
> 
> Why do we need both the WorkerTest.done() variant and the workerTestDone
> variant?  Seems like a lot of code duplication for no obvious reason.

Oops.  WorkerTest was supposed to be deleted.  Its actually commented out in the patch.  I originally was using WorkerTest, but switched to straight functions to avoid polluting the global.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Test DOM interfaces exposed on worker global. (v2)

Removed stale code and fixed other formatting nits.

In regard to workerTestGetPrefs(), I chose to echo back the requested prefs array in the response.  The worker listener then ignores any messages with the wrong list of prefs.  I added this for workerTestGetPermissions() as well.

Finally, I added a type parameter to all of the main-thread-to-worker response messages to avoid any cross-talk between the functions.

Try build:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=38311d0394a5

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cfa55304cc0

Comment 6

[(no longer active)]

Yay!  Thanks for doing this, Ben!

Comment 7

[Boris Zbarsky [:bzbarsky]]

>+    {name: "Infinity"},

Just "Infinity" would work...

Btw, this caught a bug which I'll likely fix in 1017988: Console is exposed in workers!

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #7)
> >+    {name: "Infinity"},
> 
> Just "Infinity" would work...
> 
> Btw, this caught a bug which I'll likely fix in 1017988: Console is exposed
> in workers!

That's not a bug; console works find in workers.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #7)
> >+    {name: "Infinity"},
> 
> Just "Infinity" would work...

Ah, well I already pushed to inbound.  Since this still works I'm inclined to leave it and we can clean it up in a later patch.

(In reply to :Ms2ger from comment #8)
> (In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #7)
> > Btw, this caught a bug which I'll likely fix in 1017988: Console is exposed
> > in workers!
> 
> That's not a bug; console works find in workers.

But is console available in workers in the spec or in other browsers?

Comment 10

[Boris Zbarsky [:bzbarsky]]

self.console works in workers.  self.Console should not be exposed until we decide to expose it, just like it's not exposed on window and not exposed in other browsers.  Console is marked as ChromeOnly for the interface object, but of course workers ignore that right now.  The patches in bug 1017988 fix that, fwiw.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8cfa55304cc0

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8cfa55304cc0