1030318 - [e10s] Enable devtools/framework tests with e10s

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

They are currently all disabled in the browser.ini file

Comment 1

[Brian Grinstead [:bgrins]]

There is an error in browser_keybindings.js:

 0:19.83 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_keybindings.js | uncaught exception - NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFocusManager.getFocusedElementForWindow] at chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1528
 0:19.83 Stack trace:
 0:19.83     JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1345
 0:19.83     JS frame :: chrome://mochikit/content/mochitest-e10s-utils.js :: e10s_init/< :: line 74
 0:19.83     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
 0:19.83

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: framework-e10s-pt1.patch

All tests except for 1 worked without changes.  Pushed to try (but will this even run e10s for devtools on try?): https://tbpl.mozilla.org/?tree=Try&rev=fe822f88002d

Comment 3

[Joe Walker [:jwalker]]

I asked in #ateam how to push to try and have mochitest-dt run in e10s mode. AFAICT it's not possible using the syntax on trychooser (no reply yet).

Mochitest is run for some parts in e10s mode on linux. See the M-e10s(1 2 3 4 5) parts of tbpl.

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c387b619bf42

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c387b619bf42

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: framework-e10s-pt2.patch

So this test failed because of the waitForFocus call (Comment 1) and this patch 'fixes' it by removing the call.  It seems to work, but it isn't a great solution to the problem (it was waiting for focus for a reason).

This function is used pretty widely across our tests so I imagine this question will come up a lot - is there a good way to use it with e10s?

Comment 7

[Tomislav Jovanovic :zombie]

hey Brian, can you point me to where the waitForFocus() is implemented?

Comment 8

[Tomislav Jovanovic :zombie]

didn't intend do clear info..

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Tomislav Jovanovic [:zombie] from comment #7)
> hey Brian, can you point me to where the waitForFocus() is implemented?

Sure, it is here: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#692.

Comment 10

[Brian Grinstead [:bgrins]]

And the issue can be seen by running:

mach mochitest-devtools --e10s browser/devtools/framework/test/browser_keybindings.js

Comment 11

[Tomislav Jovanovic :zombie]

so, at first look, i though this would be close to impossible to hack the helper waitForFocus() to work in chrome context, without moving all that logic to the frame script (where it would probably have to be reused via a JSM or something), because of the event listeners. 

but after some digging, it seems adding event listeners to DOM CPOWs now works, after bug 996785 was fixed (good to know! ;).

the second thing, that is actually throwing is the nsIFocusManager, obviously because it doesn't expect, and can't deal with CPOWs. unfortunately, and i'm not that familiar with focus-related APIs in gecko, but a quick look suggests it might be possible to get the similar information using the standard DOM APIs like document.hasFocus() and document.activeElement.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Tomislav Jovanovic [:zombie] from comment #11)
> so, at first look, i though this would be close to impossible to hack the
> helper waitForFocus() to work in chrome context, without moving all that
> logic to the frame script (where it would probably have to be reused via a
> JSM or something), because of the event listeners. 
> 
> but after some digging, it seems adding event listeners to DOM CPOWs now
> works, after bug 996785 was fixed (good to know! ;).
> 
> the second thing, that is actually throwing is the nsIFocusManager,
> obviously because it doesn't expect, and can't deal with CPOWs.
> unfortunately, and i'm not that familiar with focus-related APIs in gecko,
> but a quick look suggests it might be possible to get the similar
> information using the standard DOM APIs like document.hasFocus() and
> document.activeElement.

Thanks for the info, I did some more searching around on the nsIFocusManager (getFocusedElementForWindow) issue and found Bug 921935.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: framework-e10s-pt2.patch

Mike, this fixes the following tests:

browser/devtools/framework/test/browser_keybindings.js
browser/devtools/framework/test/browser_toolbox_options_disable_js.js
browser/devtools/framework/test/browser_target_events.js

And it starts trying to fix the disable cache one but there are still multiple problems.  Firstly, the assertions fail and as far as I can tell the cache actually isn't getting disabled.  Secondly, there is a hangup during the destroy process that causes the options panel destroy promise to never resolve (I think due to a call to activeTab.reconfigure).  So I've left a few of the basic fixes in there and disabled it for e10s in the browser.ini:
browser/devtools/framework/test/browser_toolbox_options_disable_cache.js

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: framework-e10s-pt2.patch

Had to move the pickerButton cleanup into then() inside of destroyInspector() to resolve a few oranges outside of framework.  Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6ca4b6d17eb4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/977a99c032e7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8468444 [details] [diff] [review]
framework-e10s-pt2.patch

Not sure if the [leave open] is needed anymore or not. Please remove it if it isn't.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> Comment on attachment 8468444 [details] [diff] [review]
> framework-e10s-pt2.patch
> 
> Not sure if the [leave open] is needed anymore or not. Please remove it if
> it isn't.

Still needed for one more test.

Comment 18

[Brian Grinstead [:bgrins]]

Mike, can you take a look at the disable caching feature and let me know if it seems to be failing in e10s mode?  To me, the assertions indicate that maybe it is not actually disabling the cache.  With the latest fx-team you can run:

mach mochitest-devtools --e10s browser/devtools/framework/test/browser_toolbox_options_disable_cache.js

Here are the errors I see:

139 INFO Passed:  11
140 INFO Failed:  4
141 INFO Todo:    1

151 WARNING The following tests failed:
152 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 0 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 0 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
TEST-INFO | expected PASS
153 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 1 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 1 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
TEST-INFO | expected PASS
154 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
TEST-INFO | expected PASS
155 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/977a99c032e7

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The test is failing just because scrollIntoView needs at least two executeSoons when using e10s... not sure why because we are not touching content there.

I have logged 1052109 for the platform guys to look into this and we can use whichever API they come up with.

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: framework-e10s-pt2.patch

Try:
https://tbpl.mozilla.org/?tree=Try&rev=43cc4d095f76
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43cc4d095f76

Comment 22

[Brian Grinstead [:bgrins]]

Pushed patch to try with e10s tests enabled: https://tbpl.mozilla.org/?tree=Try&rev=b6717f7e7304

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8485734 [details] [diff] [review]
framework-e10s-pt2.patch

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

Looks like some good refactors overall.  Unfortunately I'm still seeing errors on the try push with e10s enabled for options-disable-cache.js: https://tbpl.mozilla.org/php/getParsedLog.php?id=47602373&tree=Try&full=1

::: browser/devtools/framework/test/head.js
@@ +137,5 @@
> +  return def.promise;
> +}
> +
> +/**
> + * element.scrollIntoView() does not fire an event on scroll... event the scroll

looks like a typo with 'event the scroll event'

Comment 24

[Brian Grinstead [:bgrins]]

Applying this patch in the queue will run tests on try in e10s mode: https://bug1061904.bugzilla.mozilla.org/attachment.cgi?id=8482977

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> Applying this patch in the queue will run tests on try in e10s mode:
> https://bug1061904.bugzilla.mozilla.org/attachment.cgi?id=8482977

It will if "--e10" is corrected to "--e10s"

Comment 26

[Chris Peterson [:cpeterson]]

Moving DevTools test bugs to e10s milestone M7 (i.e. not blocking e10s merge to Aurora).

Comment 27

[Eddy Bruel [:ejpbruel]]

Hey Brian, how are we doing with this bug? Is there anything blocking you from finishing this up?

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: framework-e10s-pt3.patch

This is just enabling tests that are now working in e10s in the browser.ini.  Bug 1070837 is no longer a problem, but here are the tests that still need to be fixed after this patch:

browser_toolbox_options_disable_cache-02.js
browser_toolbox_options_disable_js.js
browser_toolbox_select_event.js

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f33b0b6d36a

Comment 29

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #28)
> 
> here are the tests that still need
> to be fixed after this patch:
> 
> browser_toolbox_options_disable_cache-02.js
> browser_toolbox_options_disable_js.js
> browser_toolbox_select_event.js

Mike, I believe the scrollIntoView thing you mentioned in Comment 20 is no longer an issue for browser_toolbox_options_disable_cache.js.  Can you check if it is still an issue in browser_toolbox_options_disable_cache-02.js, or if this is a different problem?  Here is the error I see locally:

142 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_cache-02.js | Tab 2 cache is not enabled - Didn't expect c69482c1-e04d-8118-89d7-a23359471b3f, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:855
    chrome://mochitests/content/browser/browser/devtools/framework/test/helper_disable_cache.js:checkCacheEnabled:64
    self-hosted:InterpretGeneratorResume:677
    self-hosted:next:585
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37
143 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_cache-02.js | Tab 2 cache is not enabled - Didn't expect c69482c1-e04d-8118-89d7-a23359471b3f, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:855
    chrome://mochitests/content/browser/browser/devtools/framework/test/helper_disable_cache.js:checkCacheEnabled:64
    self-hosted:InterpretGeneratorResume:677
    self-hosted:next:585
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37
SUITE-END | took 10s

Comment 30

[Brian Grinstead [:bgrins]]

Attachment: framework-e10s-pt3.patch

OK, so apparently browser_toolbox_options_disable_cache-01.js wasn't safe to enable for linux (failures here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f33b0b6d36a).

Keeping that disabled for now - here is a new try push with this version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b2c3fe0321

Comment 31

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8550048 [details] [diff] [review]
framework-e10s-pt3.patch

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

Do we really need reviews for patches like this? I would be fine with r=me for similar patches in the future (i.e. trivial patches that only touch a configuration file).

Comment 32

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/700f4ab5423a

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/700f4ab5423a

Comment 34

[Neil Deakin]

Note that bug 1083269 fixed waitForFocus to work with content process windows. Does this affect any on the tests disabled or changed here?

Comment 35

[Brian Grinstead [:bgrins]]

(In reply to Neil Deakin from comment #34)
> Note that bug 1083269 fixed waitForFocus to work with content process
> windows. Does this affect any on the tests disabled or changed here?

I don't think so.. In Attachment 8485734 [details] [diff] I changed a call from waitForFocus(setupKeyBindingsTest, content) to waitForFocus(setupKeyBindingsTest) and everything seems to work, so unless if there is a reason to undo that change we should be fine here.

Comment 36

[Brian Grinstead [:bgrins]]

Attachment: framework-e10s-pt4.patch

Mike, I think the ni? from earlier got cleared.  See Comment 29 - the following 3 tests are still failing in e10s (and don't seem related to the scrollIntoView issue).  AFAICT from the error messages, the disable cache/js features may not be working in e10s.  Since you've worked on these features, can you take a look and see what is going on, at least to help narrow down where things are breaking here?

browser_toolbox_options_disable_cache-01.js
browser_toolbox_options_disable_cache-02.js
browser_toolbox_options_disable_js.js

Comment 37

[Eddy Bruel [:ejpbruel]]

I think we should try to get these tests into the Aurora 40 uplift, so I propose upgrading this bug to M6.

Comment 38

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #36)
> Created attachment 8576965 [details] [diff] [review]
> framework-e10s-pt4.patch
> 
> Mike, I think the ni? from earlier got cleared.  See Comment 29 - the
> following 3 tests are still failing in e10s (and don't seem related to the
> scrollIntoView issue).  AFAICT from the error messages, the disable cache/js
> features may not be working in e10s.  Since you've worked on these features,
> can you take a look and see what is going on, at least to help narrow down
> where things are breaking here?
> 
> browser_toolbox_options_disable_cache-01.js
> browser_toolbox_options_disable_cache-02.js
> browser_toolbox_options_disable_js.js

I will take a look.

Comment 39

[Jim Mathies [:jimm]]

Hey brian, how's this work coming along? We're about two weeks away from uplift.

Comment 40

[Brian Grinstead [:bgrins]]

(In reply to Jim Mathies [:jimm] from comment #39)
> Hey brian, how's this work coming along? We're about two weeks away from
> uplift.

It's just the disable cache / disable js tests remaining.  I can't tell if the feature itself isn't working or if it's a test-only issue.  Mike knows this feature well so I think he was going to take a look at it, forwarding the needinfo.

Comment 41

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1030318-scrollintoview-needs-wait.patch

(In reply to Brian Grinstead [:bgrins] from comment #36)
> Created attachment 8576965 [details] [diff] [review]
> framework-e10s-pt4.patch
> 
> Mike, I think the ni? from earlier got cleared.  See Comment 29 - the
> following 3 tests are still failing in e10s (and don't seem related to the
> scrollIntoView issue).

This patch ads a 500ms wait between cbx.scrollintoview() and cbx.click()... suddenly the tests pass locally in e10s mode.

It may be that we will need to up this number on try but 100ms works locally.

In bug 1052109 we request a way to reliably wait for scrollintoview but it is hard, especially inside nested iframes. For the moment adding the pause seems like the only answer we have.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03a2341cd650

Comment 42

[Brian Grinstead [:bgrins]]

Comment on attachment 8605749 [details] [diff] [review]
1030318-scrollintoview-needs-wait.patch

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

I've been bit by this bug before also, but I thought calling el.click() was safe regardless of whether it was in view (as opposed to synthesizemouse).  Can you test locally and see if putting the `wait` after the checkbox click (instead of before) still fixes it?  Because I suspect we can actually remove the call to scrollIntoView completely and the tests would still work.

r=me if try is green and if the problem is in fact the scrollIntoVeiw issue.  Make sure to trigger a bunch of re-runs on the e10s tests so we can catch any likely intermittents

::: browser/devtools/framework/test/browser_toolbox_options_disable_js.js
@@ +62,5 @@
>    let panel = toolbox.getCurrentPanel();
>    let cbx = panel.panelDoc.getElementById("devtools-disable-javascript");
>  
>    cbx.scrollIntoView();
> +  waitFor(500).then(() => {

Please add a comment linking to the relevant bug that will let us remove this.

::: browser/devtools/framework/test/helper_disable_cache.js
@@ +73,5 @@
>  
>    cbx.scrollIntoView();
>  
>    // After uising scrollIntoView() we need to wait for the browser to scroll.
> +  yield waitFor(500);

Ditto

Comment 43

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1030318-scrollintoview-needs-wait.patch

- No longer using scrollIntoView().
- Uses cbx.click() as requested.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=673f76cd2085

Comment 44

[Jim Mathies [:jimm]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> Created attachment 8607547 [details] [diff] [review]
> 1030318-scrollintoview-needs-wait.patch
> 
> - No longer using scrollIntoView().
> - Uses cbx.click() as requested.
> 
> Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=673f76cd2085

Looks like you have a couple failures here, but nothing that looks too serious - 

695 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_js.js | Output is "JavaScript Enabled" - Got No output, expected JavaScript Enabled
697 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_js.js | Output is "JavaScript Enabled" in iframe - Got No output, expected JavaScript Enabled 

Curious, what's the current eta on finishing this bug up?

Comment 45

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Jim Mathies [:jimm] from comment #44)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> > Created attachment 8607547 [details] [diff] [review]
> > 1030318-scrollintoview-needs-wait.patch
> > 
> > - No longer using scrollIntoView().
> > - Uses cbx.click() as requested.
> > 
> > Try:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=673f76cd2085
> 
> Looks like you have a couple failures here, but nothing that looks too
> serious - 
> 
> 695 INFO TEST-UNEXPECTED-FAIL |
> browser/devtools/framework/test/browser_toolbox_options_disable_js.js |
> Output is "JavaScript Enabled" - Got No output, expected JavaScript Enabled
> 697 INFO TEST-UNEXPECTED-FAIL |
> browser/devtools/framework/test/browser_toolbox_options_disable_js.js |
> Output is "JavaScript Enabled" in iframe - Got No output, expected
> JavaScript Enabled 
> 
> Curious, what's the current eta on finishing this bug up?

I have no time at the moment to investigate other approaches so I guess we have three options:
1. Leave it for now as the tests are currently disabled and in real life the features work perfectly.
2. Wait for me to finish the storage inspector when I can take another look.
3. Somebody else can take a look.

We probably just need to add a delay but it would be good to know exactly what is making the pause necessary. Maybe we should be waiting a little longer after toolbox init?

Comment 46

[Brian Grinstead [:bgrins]]

I've got a try push here that enables the disable cache / js tests originally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6074ad0bd6fc.  There are two others I'll leave disabled but since we have bugs already for them (bug 1231869 and bug 1069044) I'm going to land that part.

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/91ba81f7207e

Comment 48

[Carsten Book [:Tomcat]]

hi, sorry had to back this out for causing perma failures on linux x64 opt like https://treeherder.mozilla.org/logviewer.html#?job_id=6742280&repo=fx-team

Comment 49

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/b2e729decd54

Comment 50

[Neil Deakin]

Looks like 1240509 would fix the failures.

Comment 51

[Neil Deakin]

Or I should say 1240368, checked in yesterday.

Comment 52

[Brian Grinstead [:bgrins]]

(In reply to Neil Deakin from comment #51)
> Or I should say 1240368, checked in yesterday.

I think this was checked in after that bug.  But here's a new try push anyway just to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13759e8ff5b6.  Guessing we'll need to wait for a resolution in Bug 1240509 before relanding

Comment 53

[Brian Grinstead [:bgrins]]

New try push on top of Bug 1204174.. maybe this will fix the perf perma orange problem: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ca792e34456

Comment 54

[Brian Grinstead [:bgrins]]

The perma orange seems gone in https://treeherder.mozilla.org/#/jobs?repo=try&revision=10aef49878b3, going to try re-landing

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d23494dca1d5

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d23494dca1d5