Closed Bug 1442313 Opened 2 years ago Closed 2 years ago

Expose getJSTestingFunctions() on WorkerGlobalScope in tests only

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(5 files)

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.
(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?
Priority: -- → P3
> What would be your ideal timeframe on this?

About when my build with the patches finishes compiling.  ;)
Assignee: nobody → bzbarsky
MozReview-Commit-ID: HTs3RwEOmZ4
Attachment #8955233 - Flags: review?(bobbyholley)
MozReview-Commit-ID: 5h51oqaf0R
Attachment #8955234 - Flags: review?(bkelly)
MozReview-Commit-ID: LiErRvJ6CTH
Attachment #8955235 - Flags: review?(bkelly)
MozReview-Commit-ID: IUQQX3ixkgl
Attachment #8955236 - Flags: review?(luke)
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?
Attachment #8955233 - Flags: review?(bobbyholley) → review+
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.
Attachment #8955236 - Flags: review?(luke) → review+
Attachment #8955234 - Flags: review?(bkelly) → review+
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?
Attachment #8955235 - Flags: review?(bkelly) → review+
> 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?
Flags: needinfo?(bkelly)
(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);
Flags: needinfo?(bkelly) → needinfo?(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.)
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
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
Backout by apavel@mozilla.com:
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
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
Flags: needinfo?(bzbarsky)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
I am 99% sure that those crashes were caused by bug 1442344 and not this bug.  Why did you decide it was this bug?
Flags: needinfo?(apavel)
We noticed the failures on mozilla-central and inbound, backfilled and retrigered on inbound, led to the culprit being your push.
Flags: needinfo?(apavel) → needinfo?(aryx.bugmail)
Yes, it was my push, but it had fixes for two bugs in it.  And I believe you backed out the wrong one.
Flags: needinfo?(apavel)
Ok. if you are sure, we can reland and backout the correct push.
Flags: needinfo?(apavel)
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.
Flags: needinfo?(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?
Flags: needinfo?(apavel)
Pushed by bzbarsky@mozilla.com:
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
Flags: needinfo?(bzbarsky)
Flags: needinfo?(aryx.bugmail)
Fixed in comment 24...
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.