Closed Bug 1386807 Opened 7 years ago Closed 6 years ago

Ship version 2 of WebCompat Go Faster addon

Categories

(Web Compatibility :: Interventions, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: miketaylr, Assigned: denschub)

References

Details

Attachments

(4 files, 4 obsolete files)

Just creating a tracking bug to hang deps/features off of this. Useful for people wanting to track this. :)
Depends on: 1386640
Depends on: 1316668, 1338586, 1383989
Summary: Ship version 1.3 of WebCompat Go Faster addon → Ship version 2 of WebCompat Go Faster addon
Flags: needinfo?(dschubert)
Depends on: 1404463
Adjusted dependencies to match the actual goal as discussed (we lacked proper written documentation on that so far).
No longer depends on: 1316668, 1338586
Depends on: 1437047
Removed bug 1386640 as a dependency, as we have discussed with Mike back then. We have a working patch in that bug, which can be shipped in an update if we need it, but as of right now, we're going to focus on JS/CSS injections and UA overrides.
No longer depends on: 1386640
Assignee: nobody → dschubert
Attachment #8949786 - Attachment is obsolete: true
Attachment #8949787 - Attachment is obsolete: true
Attachment #8949788 - Attachment is obsolete: true
Attachment #8949789 - Attachment is obsolete: true
Actually, dropping those patches. They were in the order of our changes on GitHub, where we indeed added ChromeUtils at a later point, simply because the changes happened at a time where other patches are already done. I wanted to roll with it anyway, but the reviewbot dislikes this approach very much, since it reviews each patch individually, so it cannot see we fixed the remarks at a later point already.

Because I do not want to submit all the changes as a large single-patch to make reviewing the patch easier, I'm going to do some Git magic to generate patches with the ChromeUtils fix already in place, so the review bot does not complain. It's a little bit more work, but justifiable in this case, as it makes reviewing *a lot* easier. And less noisy.
Attachment #8949819 - Flags: review?(felipc)
Attachment #8949820 - Flags: review?(felipc)
Attachment #8949821 - Flags: review?(felipc)
Felipe, I hope you do not mind if I ask you to review the latest WebCompat GoFaster addon again! :) As usual, I divided the patch into multiple parts, so that they are easier to review.

Part 1 (tracked in bug 1383989) allows us to write UA overrides targetting either Fennec or Desktop. Right now, our code isn't even in Fennec, but that's one of the tasks I am going to work on next. Since we only want to maintain one codebase with as little as much differences for both platforms, you'll see that this patch adds the Fennec metadata to install.rdf. When running on Desktop, this information is simply ignored, so I hope this is not an issue!

Part 2 (tracked in bug 1404463) creates some bootstrapping for us to allow shipping JS and CSS injections to deploy site hofixes. We decided to leverage the powers of webextensions here, so this patch includes the work needed to ship a webextension embedded inside a system extension. As I said, this is only creating a framework that allows us to ship patches via the GoFaster update mechanism. The only actual site patch shipped with this patch is affecting webcompat-addon-testcases.schub.io. As in the previous version, this dummy site is used by QA to verify that the extension works as planned.

Part 3 (tracked in bug 1437047) fixes a regression we introduced in the previous version. Because of that, the preference to turn off User Agent overrides (`extensions.webcompat.perform_ua_overrides`) had no effects. This patch is fixing that bug by returning early in the user agent override-callback when we are not supposed to do anything.

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e8af8fef18c Please note that there is one failed job: the eslint job. I already fixed this remark by adding a "/* globals browser */" to the file before submitting this review request, but did not start another try job. If you want to see a clean try run, let me know.

If you have any more questions or concerns, please let me know. I'll update this bug when I have news regarding QA, likely on Monday since it's already really late in the EU. :)
Comment on attachment 8949820 [details]
Bug 1386807 - Part 2: Add embedded webextension to enable CSS and JS overrides

https://reviewboard.mozilla.org/r/219130/#review225740

::: browser/extensions/webcompat/webextension/injections/js/bug0000000-dummy-js-injection.js:4
(Diff revision 2)
> +"use strict";
> +
> +// eslint-disable-next-line no-eval
> +window.eval(`(function() {

This is unacceptable. Aside from the fact that it will fail on sites with CSPs that block eval, we're probably going to tighten the rules for how eval can be used in content scripts in the near future, and this code may not survive.
Comment on attachment 8949819 [details]
Bug 1386807 - Part 1: Allow platform-specific UA overrides

https://reviewboard.mozilla.org/r/219128/#review225758
Attachment #8949819 - Flags: review?(felipc) → review+
Comment on attachment 8949820 [details]
Bug 1386807 - Part 2: Add embedded webextension to enable CSS and JS overrides

https://reviewboard.mozilla.org/r/219130/#review225760

(as talked on IRC, I don't have enough experience with Webextensions, so this should be reviewed by someone else)
Attachment #8949820 - Flags: review?(felipc)
Comment on attachment 8949821 [details]
Bug 1386807 - Part 3: Make the UA pref actually disable UA overrides

https://reviewboard.mozilla.org/r/219132/#review225762
Attachment #8949821 - Flags: review?(felipc) → review+
Thanks for jumping in, Kris! Do you have alternative solutions for running scripts in the website's context? According to mdn ([0]), using eval() is still the only option. Also, for my own curiosity, is there a tracking bug for these changes? I found bug 1437867, but this is seems to be only about sandboxing options for the new userScripts API.

Thanks for the reviews, Felipe!

[0]: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts#Using_eval()_in_content_scripts
Status: NEW → ASSIGNED
Flags: needinfo?(kmaglione+bmo)
(In reply to Dennis Schubert [:denschub] from comment #21)
> Thanks for jumping in, Kris! Do you have alternative solutions for running
> scripts in the website's context? According to mdn ([0]), using eval() is
> still the only option.

Object.defineProperty(window.wrappedJSObject, "isTestFeatureSupported", ...);

> Also, for my own curiosity, is there a tracking bug for these changes?

Not at the moment, no.
Flags: needinfo?(kmaglione+bmo)
> Object.defineProperty(window.wrappedJSObject, "isTestFeatureSupported", ...);

Well, yeah, sure. That will work in this specific case, you are right. Zhis is only our QA test case, but we need something that we can use in other cases. Ultimately, I can imagine a lot of past webcompat issues which only could be fixed by running complex scripts inside the pages context, so this won't be a solution for everything...

I'll change our test case to an implementation without eval, but not being able to use it without real alternatives (until the userScript API is landed, that is), is kinda bad. Will have to investigate and find something for us to use in actual page fixes...
(In reply to Dennis Schubert [:denschub] from comment #23)
> > Object.defineProperty(window.wrappedJSObject, "isTestFeatureSupported", ...);
> 
> Well, yeah, sure. That will work in this specific case, you are right. Zhis
> is only our QA test case, but we need something that we can use in other
> cases. Ultimately, I can imagine a lot of past webcompat issues which only
> could be fixed by running complex scripts inside the pages context, so this
> won't be a solution for everything...
> 
> I'll change our test case to an implementation without eval, but not being
> able to use it without real alternatives (until the userScript API is
> landed, that is), is kinda bad. Will have to investigate and find something
> for us to use in actual page fixes...

When you find a case that you can't handle without eval, let me know, and I'll give you alternatives.

But, like I said, even now `eval` won't work on pages with reasonably strict CSPs.
(That said, I also don't see how the userScript API would help you with this at all)
> When you find a case that you can't handle without eval, let me know

Well, actually, even out test case will not work, as I just realized. When I run `Object.defineProperty` in the content_script's scope, then, well, the object is defined in the content script's scope. Accessing it from inside the web page results in a permission error. (Just to make sure, I actually changed the content script and confirmed that it indeed throws an error on accessing)

The only way of getting around that I can think of, right now, is using eval. It's possible that I am missing something, so please help me out.
Flags: needinfo?(kmaglione+bmo)
Kris, what's ultimately needed is a way to alter the window object BEFORE the page scripts have a chance to run. Right now the only way to do that without scope-crossing issues is to use window.(wrappedJSObject).eval in a pre-registered content script (dynamically or statically), or to listen to webRequests and rewrite them to inject our own script using filterResponseData (which is frankly insane).

Unfortunately, relying on script-tag injection or browser.tabs.executeScript does not currently suffice, as they do not run early enough to alter the window object in advance of at least inline page-scripts. As such with them alone it's impossible to even get a fully-functional user agent spoofing addon working on all pages, since they won't always hook in early enough before a page can read navigator.userAgent.

So if anything, I'd encourage us to allow window.eval DESPITE the page's CSP, since even the CSP spec is clear that user addons are meant to not be affected by CSP policies. To me, eval is no worse than the export helpers we intentionally added in bug 1280482, or allowing users to call executeScript from the background script with a string value for the code to run.

It's wrappedJSObject that makes me worry, due to the potential it would seem to create for objects to bleed across scopes (or at least to allow things that only pointlessly break, as Dennis also discovered in comment 26). It strikes me as a bit of a footgun.
(In reply to Dennis Schubert [:denschub] from comment #26)
> Well, actually, even out test case will not work, as I just realized. When I
> run `Object.defineProperty` in the content_script's scope, then, well, the
> object is defined in the content script's scope. Accessing it from inside
> the web page results in a permission error. (Just to make sure, I actually
> changed the content script and confirmed that it indeed throws an error on
> accessing)
> 
> The only way of getting around that I can think of, right now, is using
> eval. It's possible that I am missing something, so please help me out.

This will work:

Object.defineProperty(window.wrappedJSObject, "isTestFeatureSupported", {
  get: exportFunction(function() {
    return true;
  }, window),

  set: exportFunction(function() {}, window)
});
Panos, as I understand it, exportFunction will only encourage bad practices as well. For instance, you will need to pass in some state (and often manage it later), like which userAgent string to use:

    setCurrentUA(UAString) {
      Object.defineProperty(window.wrappedJSObject.navigator, "userAgent", {
        get: exportFunction(function() {
               return UAString;
             }, window)
      });
    }

But what happens when the addon/content script closure with UAString goes away (addon is disabled, updates, etc)? Any page scripts calling navigator.userAgent will trigger a subtle exception "can't access dead object", with no useful stack trace information. If you're lucky the addon will restart in time to avoid this, or the addon author will be skilled enough to know an obscure way to "unset" the effects of the defineProperty before the addon goes away (but I would not want to rely on either of those things being true).

And so I honestly don't think we should be discouraging one bad practice (eval) in favor of another. I think we should drop all of these APIs in favor of a new one that encourages better practices by default in its design, and:

- ensures no cross-scope variable bleeding can happen
- ensures that the code being run is inaccessible from the page script by default
- allows the user to update the anonymous closure's state by sending it safe messages.

For instance, an API that content scripts can run, `browser.executeFunctionInPageScript(function, arguments)`, which ensures that the function and arguments are passed in without crossing scopes (structured cloning the arguments, etc), and runs the function in an anonymous closure in the page scripts so that it's hidden from the rest of the page scripts by default.
Attachment #8949820 - Flags: review?(felipc)
Panos, thanks for that. You are right, it works in this specific case and replaces eval() for this test case. I changed the case so we can land it. If, in the future, we run into issues where we don't see a solution, we can circle back and have discussions on this.

Try run for the new code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0912afef7fc6968790aebb9a55d9ab3e99d7ba5

Kris, is this solution okay for you? And if so, do you mind if I r? you here?
I forgot to enable Linux-64 to have Talos running, so here is another try run for all platforms with Talos, just to be sure we don't accidentally make everything slower: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b9082313ef92e711e9007925c4a6ad3826bc1aa

Looks good to me. The one busted Talos Stylo sequencial build seems to be a known failure and not caused by me. The rest are also just known failures.
Note that you want to retrigger the talos runs 5 times to get statistically significant comparisons.
Depends on: 1444192
Priority: -- → P1
(In reply to Panos Astithas [:past] (please ni?) from comment #34)
> Note that you want to retrigger the talos runs 5 times to get statistically
> significant comparisons.

Dennis, can you re-run the talos tries x5? Thanks.
Flags: needinfo?(dschubert)
Since I run into some permission issues with treeherder, I pushed again with `--rebuild-talos 5` so we have results before I got the permission thing sorted out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe4c024b27cfb318e4af57af1a1b2ce37fe200a1
Flags: needinfo?(dschubert)
Attachment #8949820 - Flags: review?(kmaglione+bmo)
So, it looks like we have a little bit of a startup performance inpact here. People in #perf were a little bit confused about the results as they are all over the place, but gave me some valuable hints on how to figure out what's going on here. Will update this thread as soon as possible.
Comment on attachment 8949820 [details]
Bug 1386807 - Part 2: Add embedded webextension to enable CSS and JS overrides

https://reviewboard.mozilla.org/r/219130/#review236492

It's probably worth noting that we're deprecating embedded WebExtensions in bootstrapped add-ons (along with bootstrapped add-ons in general) in favor of WebExtensions with embedded privileged APIs.

::: browser/extensions/webcompat/webextension/background.js:23
(Diff revision 3)
> +  Promise.all(registrations).then((contentScriptHandles) => {
> +    registeredContentScripts = contentScriptHandles;
> +  }).catch((ex) => {

It would probably be better to try-catch each of the promises individually, or if any of the scripts fail to register, you won't be able to cleanup any of the ones that succeed.

Also, we tend to prefer async functions to explicit promise .then()/.catch() calls these days.

::: browser/extensions/webcompat/webextension/manifest.json:10
(Diff revision 3)
> +  "version": "2.0",
> +
> +  "applications": {
> +    "gecko": {
> +      "id": "webcompat@mozilla.org",
> +      "strict_max_version": "99.*",

Unnecessary. Just leave this out and it defaults to "*".
Attachment #8949820 - Flags: review?(kmaglione+bmo) → review+
Depends on: 1451022
Updated the patch to address performance issues. Latest try run is looking fine, some reftests failed due to a timeout caused by a horrible job backlog yesterday, but they passed on other runs. All other failures are intermittent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d580fdba556eb6876827e08da2180efc71584252

Talos is also looking fine: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=d580fdba556eb6876827e08da2180efc71584252&framework=1&showOnlyImportant=1&selectedTimeRange=172800 - All significant changes are well within their usual deviation, as I've been made aware by #perf.

Kris, I had to delay the webextension startup by a bit, and although this code was inspired by mconley and got internal team review, I'd highly appreciate if you have a quick look at it, if you can. The interdiff is at https://reviewboard.mozilla.org/r/219130/diff/4-5/. I'll be done after that. :)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Blocks: 1452707
Hey Mike, as discussed in IRC... could you quickly take a look at this interdiff https://reviewboard.mozilla.org/r/219130/diff/4-5/ just to make sure I didn't to anything wonky here? I shouldn't, but it feels wrong to ship code that has been modified after the r+. :) Thanks much!
Flags: needinfo?(kmaglione+bmo) → needinfo?(mconley)
Comment on attachment 8949820 [details]
Bug 1386807 - Part 2: Add embedded webextension to enable CSS and JS overrides

https://reviewboard.mozilla.org/r/219130/#review244220

Seems good to me. Thanks DenSchub!

::: browser/extensions/webcompat/bootstrap.js:111
(Diff revisions 4 - 5)
> +      Services.obs.removeObserver(this, "browser-delayed-startup-finished");
> +    }
> +  };
> +  Services.obs.addObserver(appStartupObserver, "browser-delayed-startup-finished");

You've got the other topic set as a UA_OVERRIDES_INIT_TOPIC const somewhere - maybe consider doing that for this topic as well, for consistency.
Attachment #8949820 - Flags: review+
Flags: needinfo?(mconley)
You are right, I made the change. Thank you so much for your quick feedback, much appreciated.

:bogdan_maris confirmed that "at this stage in the sign-off process the feature is GREEN from QA perspective" in a preliminary report. The full report will be submitted by the end of 27th of April, but given that the manual tests performed by QA all pass and most of the changes are covered by automated tests testing WebExtensions anyway, we should land this, as the soft freeze is coming up next week.

I don't see this as a risky patch at all, especially given all the testing and reviewing it receives, but it's always good to have this in Nightly for an additional week, just in case something really unexpected happens.

As discussed with Mike (Taylor), we'll follow up with some patches after this has been landed, like addressing bug 1444192 and also also finishing work for bug 1438532 before shipping our first actual site past, but these do not improve the functionality here, so having the current state landed is more important.
Keywords: checkin-needed
Sorry for the delay. This got lost in my backlog. In the future, please ping me if my reviews are blocking you.

Also, note that bug 1378459 should solve, or at least improve, this issue.
No worries! After I pinged you for the first review on IRC, I figured it's probably easier if I just ping mconley, since he helped me out anyway. QA approval only arrived today, so you didn't block anything! :)

We're pretty fine with out solution now, but we'll keep an eye out on bug 1378459 to see if we can adopt in the future! Thanks for the hint.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aa4d27576edb
Part 1: Allow platform-specific UA overrides r=Felipe
https://hg.mozilla.org/integration/autoland/rev/8b07ab65a690
Part 2: Add embedded webextension to enable CSS and JS overrides r=kmag,mconley
https://hg.mozilla.org/integration/autoland/rev/19cd9d4bd25d
Part 3: Make the UA pref actually disable UA overrides r=Felipe
Keywords: checkin-needed
Backed out 3 changesets (bug 1386807) for Mochitest failure on accessible/tests/mochitest/events/docload/test_docload_root.html. CLOSED TREE 

Log of the failure: 
https://treeherder.mozilla.org/logviewer.html#?job_id=174853148&repo=autoland&lineNumber=2139

21:05:40     INFO - Invoke the 'open dialog 'about:about'' test { scenario #0: expected 'reorder' event;  }
[task 2018-04-20T21:05:40.441Z] 21:05:40     INFO - TEST-PASS | accessible/tests/mochitest/events/docload/test_docload_root.html | Wrong value of property 'role' for [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]]. 
[task 2018-04-20T21:05:40.442Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 0 : ['document node', address: [object HTMLDocument], role: chrome window, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.444Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 1 : ['document node', address: [object XULDocument], role: chrome window, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.445Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 2 : ['document node', address: [object XULDocument], role: chrome window, name: 'Chrome Test Harness - Firefox Nightly', address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.446Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 3 : ['document node', address: [object HTMLDocument], role: chrome window, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.447Z] 21:05:40     INFO - Buffered messages finished
[task 2018-04-20T21:05:40.449Z] 21:05:40     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/docload/test_docload_root.html | [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] has an extra child at index 4 : ['document node', address: [object HTMLDocument], role: chrome window, name: 'About About', address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]] 
[task 2018-04-20T21:05:40.450Z] 21:05:40     INFO - testAccessibleTree@chrome://mochitests/content/a11y/accessible/tests/mochitest/common.js:494:13
[task 2018-04-20T21:05:40.451Z] 21:05:40     INFO - openDialogWnd/this.finalCheck@chrome://mochitests/content/a11y/accessible/tests/mochitest/events/docload/test_docload_root.html:82:9
[task 2018-04-20T21:05:40.452Z] 21:05:40     INFO - eventQueue_processNextInvoker@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:311:9
[task 2018-04-20T21:05:40.456Z] 21:05:40     INFO - eventQueue_processNextInvokerInTimeout/<@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:450:43
[task 2018-04-20T21:05:40.457Z] 21:05:40     INFO - TEST-PASS | accessible/tests/mochitest/events/docload/test_docload_root.html | The document accessible for 'about:about' is not in cache! 
[task 2018-04-20T21:05:40.458Z] 21:05:40     INFO - TEST-PASS | accessible/tests/mochitest/events/docload/test_docload_root.html | Test with ID = 'open dialog 'about:about'' succeed.  Event 'reorder' was handled. 
[task 2018-04-20T21:05:40.459Z] 21:05:40     INFO - Invoke the 'close dialog' test { scenario #0: expected 'focus' event;  }
[task 2018-04-20T21:05:40.459Z] 21:05:40     INFO - TEST-PASS | accessible/tests/mochitest/events/docload/test_docload_root.html | The document accessible for dialog is in cache still! 
[task 2018-04-20T21:05:40.460Z] 21:05:40     INFO - TEST-PASS | accessible/tests/mochitest/events/docload/test_docload_root.html | Test with ID = 'close dialog' succeed.  Event 'focus' was handled. 
[task 2018-04-20T21:05:40.466Z] 21:05:40     INFO - GECKO(1038) | MEMORY STAT | vsize 1040MB | residentFast 302MB | heapAllocated 105MB
[task 2018-04-20T21:05:40.466Z] 21:05:40     INFO - TEST-OK | accessible/tests/mochitest/events/docload/test_docload_root.html | took 582ms
[task 2018-04-20T21:05:40.467Z] 21:05:40     INFO - TEST-START | accessible/tests/mochitest/events/docload/test_docload_shutdown.html
[task 2018-04-20T21:05:40.576Z] 21:05:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-04-20T21:05:40.579Z] 21:05:40     INFO - Buffered messages logged at 21:05:40
[task 2018-04-20T21:05:40.582Z] 21:05:40     INFO - must wait for load
[task 2018-04-20T21:05:40.584Z] 21:05:40     INFO - Invoke the 'open dialog '../../events/docload/docload_wnd.html'' test { scenario #0: expected 'reorder' event; expected 'hide' event;  }
[task 2018-04-20T21:05:40.586Z] 21:05:40     INFO - TEST-PASS | accessible/tests/mochitest/events/docload/test_docload_shutdown.html | Wrong value of property 'role' for [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]]. 
[task 2018-04-20T21:05:40.588Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 0 : ['document node', address: [object HTMLDocument], role: chrome window, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.591Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 1 : ['document node', address: [object XULDocument], role: chrome window, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.593Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 2 : ['document node', address: [object XULDocument], role: chrome window, name: 'Chrome Test Harness - Firefox Nightly', address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.599Z] 21:05:40     INFO - Matching { app root: [ chrome window: [ ] , chrome window: [ ] , chrome window: [ ] , chrome window: [ ] ]  } and [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]] child at index 3 : ['document node', address: [object HTMLDocument], role: chrome window, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleDocument)]]
[task 2018-04-20T21:05:40.599Z] 21:05:40     INFO - Buffered messages finished

Backout:
https://hg.mozilla.org/integration/autoland/rev/a08096e187fd47f2f9380c461cc7b7d577960de5

Push with the failure:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=19cd9d4bd25d84502a749f33af79c2fd2ca59ce8
Flags: needinfo?(dschubert)
Sorry for the noise. The failing test was one that sometimes timed out, and it looked like it was just intermittent. Yikes. :(

Anyway, the failing test is known to be wonky, and bug 1440106 is going to fixes that. Work in that bug was staled after some issues with macOS, but I talked to eeejay, who will now re-land that patch with a skip for macOS hopefully tomorrow. All other solutions to make that test green would be bad workarounds, so I'm marking this as a blocker.
Depends on: 1440106
Flags: needinfo?(dschubert)
Comment on attachment 8970967 [details]
Bug 1386807 - Part 4: Add hidden WebExtension window to a11y tests to make them pass

https://reviewboard.mozilla.org/r/239718/#review245402

Good temporary fix. The try job in this reviewboard is failing, but i'm assuming part 4 is not part of it?
Attachment #8970967 - Flags: review?(eitan) → review+
(Whoops, head-on collision!)

The work in bug 1440106 got delayed a bit, and I don't want to miss the 61 train because of that. As discussed with :eeejay on IRC, I adjusted the test to accept the webextension. This is a bit of a workaround, as ideally, we should simply completely ignore them in the test itself, but that's what will be done in bug 1440106 very soon.

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e2f3ab3b1730df5ed6a04ae1b6935df7cd9f065 is looking good, but I'll retrigger some tests to make sure they are really intermittent, as I'd rather not bounce twice. Pushing the patches nonetheless, so they can be reviewed while I wait for the test results.

> The try job in this reviewboard is failing

Yes, that's actually an old one. The new one is linked in this comment. :)

Thank you for the super fast review.
Tests in question passed on retry, so it looks like we're good. Let's re-land.
No longer depends on: 1440106
Keywords: checkin-needed
See Also: → 1440106
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2b9b6042e586
Part 1: Allow platform-specific UA overrides r=Felipe
https://hg.mozilla.org/integration/autoland/rev/6cd45f41ee01
Part 2: Add embedded webextension to enable CSS and JS overrides r=kmag,mconley
https://hg.mozilla.org/integration/autoland/rev/8da96d4d931a
Part 3: Make the UA pref actually disable UA overrides r=Felipe
https://hg.mozilla.org/integration/autoland/rev/669b85a6cab8
Part 4: Add hidden WebExtension window to a11y tests to make them pass r=eeejay
Keywords: checkin-needed
Andrei, according to https://bugzilla.mozilla.org/show_bug.cgi?id=1447859, this is a known failure. I don't see how my code could affect this at all, what am I supposed to do here?
Flags: needinfo?(aciure)
I did a try run with the patch from bug 1449166 (which fixes the intermittent), and it also fixes the failure in this case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eda3651fd09a108eb2d4ea77c4f94f81883e95d

There were some global changes in bug 1456686 earlier, which caused breakage for our addon when simply rebasing this patch on top of it, as it removed a ChromeUtils.defineModuleGetter call. It was unused previously, which is why it got removed, but it's needed now. I rebased on the central and fixed that breakage, but the resulting code is exactly the same, i just re-added the import that was there previously.

Trees are closed right now, but the patch in question is already in autoland, so it should be just fine if we land this patchset, assuming bug 1449166 sticks... Leaving the ni? in place, so maybe Andrei can land it again.
Hm, Andrei is probably not on Sheriff duty today, so keeping that ni? isn't getting us anywhere. I confirmed in #developers that our backout was for ... making the intermittent more predictable, sometimes... but as visible in the try push above, the patch that fixed the intermittent also fixes the failure created here - because it was the same. So that's resolved.

As Andrei is probably not merging stuff today, I'll re-set checkin-needed...

Due to the fact we technically are already in soft-freeze, this will be my last attempt for 61. I don't see how we can cause more breakage, tests are green, there are no new changes to our sources in the meantime, should be good!
Flags: needinfo?(aciure)
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a97531cf72c3
Part 1: Allow platform-specific UA overrides r=Felipe
https://hg.mozilla.org/integration/autoland/rev/3307b23d2889
Part 2: Add embedded webextension to enable CSS and JS overrides r=kmag,mconley
https://hg.mozilla.org/integration/autoland/rev/5e1fe3a9874b
Part 3: Make the UA pref actually disable UA overrides r=Felipe
https://hg.mozilla.org/integration/autoland/rev/bb6b514d6988
Part 4: Add hidden WebExtension window to a11y tests to make them pass r=eeejay
Keywords: checkin-needed
Depends on: 1457538
You need to log in before you can comment on or make changes to this bug.