1442313 - Expose getJSTestingFunctions() on WorkerGlobalScope in tests only

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

On mainthread we can get to these via SpecialPowers, but on workers we can't do that.  If we pushed out the xpc::IsInAutomation state to workers, we could do this, I would think.

Comment 1

[Marion Daly [:mdaly]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #0)
> On mainthread we can get to these via SpecialPowers, but on workers we can't
> do that.  If we pushed out the xpc::IsInAutomation state to workers, we
> could do this, I would think.

What would be your ideal timeframe on this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> What would be your ideal timeframe on this?

About when my build with the patches finishes compiling.  ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make xpc::IsInAutomation a bit faster

MozReview-Commit-ID: HTs3RwEOmZ4

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Push down the value of xpc::IsInAutomation into workers

MozReview-Commit-ID: 5h51oqaf0R

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Expose, test-only, getJSTestingFunctions() in workers

MozReview-Commit-ID: LiErRvJ6CTH

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Use getJSTestingFunctions() in test_worker_interfaces.js

MozReview-Commit-ID: IUQQX3ixkgl

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8955233 [details] [diff] [review]
part 1.  Make xpc::IsInAutomation a bit faster

Review of attachment 8955233 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcpublic.h
@@ +676,5 @@
>  
>  inline bool
>  IsInAutomation()
>  {
> +    static bool sBlowUpSecurityPrefSet;

How about just sAutomationPrefIsSet?

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8955236 [details] [diff] [review]
part 4.  Use getJSTestingFunctions() in test_worker_interfaces.js

Review of attachment 8955236 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

::: dom/workers/test/test_worker_interfaces.js
@@ -291,5 @@
>              (isInsecureContext && !Boolean(entry.insecureContext)) ||
>              entry.disabled) {
>            interfaceMap[entry.name] = false;
> -        } else if (entry.optional) {
> -          interfaceMap[entry.name] = "optional";

I think there are two more bits of optional-handling in this file.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8955235 [details] [diff] [review]
part 3.  Expose, test-only, getJSTestingFunctions() in workers

Review of attachment 8955235 [details] [diff] [review]:
-----------------------------------------------------------------

Given that we explicitly enable this for tests like test_interfaces.js, it would be nice if there was some way to ensure it was never exposed in web-exposed content.  Is it possible to write such a test?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I think there are two more bits of optional-handling in this file.

Good point.  Fixed.

> Is it possible to write such a test?

I can't think of a way, because by definition we're exposing it in all our automation.

I could change the exposure function to restrict to the two known urls that need it, I guess, and add another test that tests that it's not there at a different URL.  Would you prefer that?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> > Is it possible to write such a test?
> 
> I can't think of a way, because by definition we're exposing it in all our
> automation.
> 
> I could change the exposure function to restrict to the two known urls that
> need it, I guess, and add another test that tests that it's not there at a
> different URL.  Would you prefer that?

Could we not simply have a test that does something like this?

  await SpecialPowers.pushPrefEnv("set",
          [["security.turn_off_all_security_so_that_viruses_can_take_over_this_computer", false]]);
  let w = new Worker('data:application/javascript,postMessage(!!self.testingFunctions)');
  let enabled = await new Promise(resolve => { w.onmessage = e => resolve(e.data) });
  is(enabled, false);

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Could we not simply have a test that does something like this?

Unfortunately, no.  We end up hitting various CrashIfNotInAutomation() calls before we get a chance to set the pref back...  (This was not obvious; I had to write the test and crash to discover it.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Failed attempt to write that test

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc2c103027b
part 1.  Make xpc::IsInAutomation a bit faster.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/9788a46b8874
part 2.  Push down the value of xpc::IsInAutomation into workers.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/660332ce1bf0
part 3.  Expose, test-only, getJSTestingFunctions() in workers.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfef9139500f
part 4.  Use getJSTestingFunctions() in test_worker_interfaces.js.  r=luke

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8fc2c103027b
https://hg.mozilla.org/mozilla-central/rev/9788a46b8874
https://hg.mozilla.org/mozilla-central/rev/660332ce1bf0
https://hg.mozilla.org/mozilla-central/rev/bfef9139500f

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/accb1b3cf593
Backed out 4 changesets for frequently failing crashtests on android, e.g. libeditor/crashtests/772282.html and layout/generic/crashtests/542136-1.html a=backout

Comment 17

[Andreea Pavel [:apavel]]

Backed out for frequently failing crashtests on android, e.g. libeditor/crashtests/772282.html and layout/generic/crashtests/542136-1.html

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b89286a885886784f8fa728e2652bed07362935c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=165537196&repo=mozilla-inbound&lineNumber=1894

Backout: https://hg.mozilla.org/mozilla-central/rev/accb1b3cf5932526b4cf983819f9fb4cc1111546

Comment 18

[Boris Zbarsky [:bzbarsky]]

I am 99% sure that those crashes were caused by bug 1442344 and not this bug.  Why did you decide it was this bug?

Comment 19

[Andreea Pavel [:apavel]]

We noticed the failures on mozilla-central and inbound, backfilled and retrigered on inbound, led to the culprit being your push.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yes, it was my push, but it had fixes for two bugs in it.  And I believe you backed out the wrong one.

Comment 21

[Andreea Pavel [:apavel]]

Ok. if you are sure, we can reland and backout the correct push.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure.  Doing a try run now to verify.

I mean, I could reland stuff myself, if you're ok with me pushing to m-c directly.

Comment 23

[Andreea Pavel [:apavel]]

I'm sure too now, because the failures keep occurring. Can you also backout and land in one push or should we do the backout from m-c?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/cb363ac1897a
part 1.  Make xpc::IsInAutomation a bit faster.  r=bholley
https://hg.mozilla.org/mozilla-central/rev/0d9766eb6f0f
part 2.  Push down the value of xpc::IsInAutomation into workers.  r=bkelly
https://hg.mozilla.org/mozilla-central/rev/193fe425f5e2
part 3.  Expose, test-only, getJSTestingFunctions() in workers.  r=bkelly
https://hg.mozilla.org/mozilla-central/rev/5e2c4277fd0c
part 4.  Use getJSTestingFunctions() in test_worker_interfaces.js.  r=luke

Comment 25

[Boris Zbarsky [:bzbarsky]]

Fixed in comment 24...