Closed
Bug 1030318
Opened 10 years ago
Closed 9 years ago
[e10s] Enable devtools/framework tests with e10s
Categories
(DevTools :: Framework, defect)
DevTools
Framework
Tracking
(e10sm6+, firefox47 fixed)
RESOLVED
FIXED
Firefox 47
People
(Reporter: bgrins, Assigned: bgrins)
References
(Depends on 1 open bug)
Details
(Whiteboard: [status:inflight])
Attachments
(6 files, 4 obsolete files)
1.05 KB,
patch
|
jwalker
:
review+
bgrins
:
checkin+
|
Details | Diff | Splinter Review |
11.43 KB,
patch
|
bgrins
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
8.85 KB,
patch
|
Details | Diff | Splinter Review | |
1.83 KB,
patch
|
ejpbruel
:
review+
bgrins
:
checkin+
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
Details | Diff | Splinter Review | |
3.60 KB,
patch
|
miker
:
review+
|
Details | Diff | Splinter Review |
They are currently all disabled in the browser.ini file
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #8446112 -
Flags: review?(jwalker) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [leave open]
Comment 3•10 years ago
|
||
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•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][fixed-in-fx-team]
Comment 5•10 years ago
|
||
Flags: in-testsuite+
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Assignee | ||
Updated•10 years ago
|
Attachment #8446112 -
Flags: checkin+
Assignee | ||
Comment 6•10 years ago
|
||
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?
Flags: needinfo?(tomica+amo)
Comment 7•10 years ago
|
||
hey Brian, can you point me to where the waitForFocus() is implemented?
Flags: needinfo?(tomica+amo)
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
And the issue can be seen by running:
mach mochitest-devtools --e10s browser/devtools/framework/test/browser_keybindings.js
Comment 11•10 years ago
|
||
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.
Flags: needinfo?(tomica+amo)
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Depends on: 921935
Updated•10 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Updated•10 years ago
|
Whiteboard: [leave open] → [leave open][status:inflight]
Assignee | ||
Comment 13•10 years ago
|
||
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
Attachment #8456377 -
Attachment is obsolete: true
Attachment #8467781 -
Flags: review?(mratcliffe)
Updated•10 years ago
|
Attachment #8467781 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Comment 14•10 years ago
|
||
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
Attachment #8467781 -
Attachment is obsolete: true
Attachment #8468444 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [leave open][status:inflight] → [leave open][fixed-in-fx-team]
Comment 16•10 years ago
|
||
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.
Attachment #8468444 -
Flags: checkin+
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 18•10 years ago
|
||
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
Flags: needinfo?(mratcliffe)
Comment 19•10 years ago
|
||
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Whiteboard: [leave open] → [leave open][status:inflight]
Comment 20•10 years ago
|
||
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•10 years ago
|
||
Try:
https://tbpl.mozilla.org/?tree=Try&rev=43cc4d095f76
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43cc4d095f76
Attachment #8485734 -
Flags: review?(bgrinstead)
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 22•10 years ago
|
||
Pushed patch to try with e10s tests enabled: https://tbpl.mozilla.org/?tree=Try&rev=b6717f7e7304
Assignee | ||
Comment 23•10 years ago
|
||
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'
Attachment #8485734 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 24•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
Moving DevTools test bugs to e10s milestone M7 (i.e. not blocking e10s merge to Aurora).
Updated•10 years ago
|
Whiteboard: [leave open][status:inflight] → [leave open][status:inflight][e10s-m7]
Comment 27•10 years ago
|
||
Hey Brian, how are we doing with this bug? Is there anything blocking you from finishing this up?
Flags: needinfo?(bgrinstead)
Updated•10 years ago
|
Whiteboard: [leave open][status:inflight][e10s-m7] → [leave open][status:inflight][e10s-m6]
Updated•10 years ago
|
Whiteboard: [leave open][status:inflight][e10s-m6] → [leave open][status:inflight][e10s-m7]
Assignee | ||
Comment 28•10 years ago
|
||
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
Flags: needinfo?(bgrinstead)
Attachment #8549899 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 29•10 years ago
|
||
(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
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 30•10 years ago
|
||
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
Attachment #8549899 -
Attachment is obsolete: true
Attachment #8549899 -
Flags: review?(ejpbruel)
Attachment #8550048 -
Flags: review?(ejpbruel)
Comment 31•10 years ago
|
||
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).
Attachment #8550048 -
Flags: review?(ejpbruel) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Whiteboard: [leave open][status:inflight][e10s-m7] → [leave open][status:inflight][e10s-m7][fixed-in-fx-team]
Comment 33•10 years ago
|
||
Whiteboard: [leave open][status:inflight][e10s-m7][fixed-in-fx-team] → [leave open][status:inflight][e10s-m7]
Assignee | ||
Updated•10 years ago
|
Attachment #8550048 -
Flags: checkin+
Comment 34•10 years ago
|
||
Note that bug 1083269 fixed waitForFocus to work with content process windows. Does this affect any on the tests disabled or changed here?
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 35•10 years ago
|
||
(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.
Flags: needinfo?(bgrinstead)
See Also: → 921935
Updated•10 years ago
|
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 36•10 years ago
|
||
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
Flags: needinfo?(mratcliffe)
Updated•10 years ago
|
Summary: Enable devtools/framework tests with e10s → [e10s] Enable devtools/framework tests with e10s
Comment 37•10 years ago
|
||
I think we should try to get these tests into the Aurora 40 uplift, so I propose upgrading this bug to M6.
Comment 38•10 years ago
|
||
(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.
Flags: needinfo?(mratcliffe)
Updated•10 years ago
|
Comment 39•10 years ago
|
||
Hey brian, how's this work coming along? We're about two weeks away from uplift.
Flags: needinfo?(bgrinstead)
Updated•10 years ago
|
Whiteboard: [leave open][status:inflight][e10s-m7] → [leave open][status:inflight]
Assignee | ||
Comment 40•10 years ago
|
||
(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.
Flags: needinfo?(bgrinstead) → needinfo?(mratcliffe)
Comment 41•10 years ago
|
||
(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
Flags: needinfo?(mratcliffe)
Attachment #8605749 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 42•10 years ago
|
||
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
Attachment #8605749 -
Flags: review?(bgrinstead) → review+
Comment 43•10 years ago
|
||
- No longer using scrollIntoView().
- Uses cbx.click() as requested.
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=673f76cd2085
Attachment #8605749 -
Attachment is obsolete: true
Attachment #8607547 -
Flags: review+
Comment 44•10 years ago
|
||
(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•10 years ago
|
||
(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?
Assignee | ||
Comment 46•9 years ago
|
||
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 48•9 years ago
|
||
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
Flags: needinfo?(bgrinstead)
Comment 49•9 years ago
|
||
Comment 50•9 years ago
|
||
Looks like 1240509 would fix the failures.
Comment 51•9 years ago
|
||
Or I should say 1240368, checked in yesterday.
Assignee | ||
Comment 52•9 years ago
|
||
(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
Depends on: 1240509
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 53•9 years ago
|
||
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
Depends on: 1204174
Assignee | ||
Comment 54•9 years ago
|
||
The perma orange seems gone in https://treeherder.mozilla.org/#/jobs?repo=try&revision=10aef49878b3, going to try re-landing
Comment 56•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•