More tests fixed.

More tests running in e10s mode.

Rebased, fixed a couple more tests, and enabled a few that were fine

Fixed a couple of more, found a weird one that passes when run alone but fails when run in the group, so I left it semicolon'd out for now with a comment in the browser.ini file

Fixed 18 additional console tests.

More tests enabled

Taking a break from working on these tests.

Fix one more test, fire events in content and not from the test.

About 10 more tests.

Fixes a few more. Getting an error from a previously-enabled test: 737 INFO Full message: TypeError: toolbox.getPanel(...) is undefined 3738 INFO Full stack: runner@chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_split_persist.js:41:21 Someone else was having problems in bug 992679 with using toolbox.getPanel("webconsole") to get a reference to the split console, when it's definitely open. Brian, any ideas there? 78 down, 118 to go

(In reply to Heather Arthur [:harth] from comment #11) > Created attachment 8477784 [details] [diff] [review] > webconsole-e10s-WIP10.patch > > Fixes a few more. Getting an error from a previously-enabled test: > > 737 INFO Full message: TypeError: toolbox.getPanel(...) is undefined > 3738 INFO Full stack: > runner@chrome://mochitests/content/browser/browser/devtools/webconsole/test/ > browser_webconsole_split_persist.js:41:21 > > Someone else was having problems in bug 992679 with using > toolbox.getPanel("webconsole") to get a reference to the split console, when > it's definitely open. Brian, any ideas there? > > 78 down, 118 to go This is weird, buttonsPromise is throwing an exception (from the _buildButtons call), which is maybe causing the promise.all call to not wait for the split console or something? If I comment the buttonsPromise line out here it passes: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#282. There seem to be two relevant errors from buildButtons (I'll file a new bug for it) 106 INFO console.error: 107 INFO Message: TypeError: window is null 108 INFO Stack: 109 INFO exports.items<.state.isChecked@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/paintflashing.js:106:15 110 INFO CommandUtils.createButtons/</</onChange@resource:///modules/devtools/DeveloperToolbar.jsm:156:27 111 INFO CommandUtils.createButtons/</<@resource:///modules/devtools/DeveloperToolbar.jsm:167:1 112 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 113 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 114 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9 115 INFO CommandUtils.createButtons/<@resource:///modules/devtools/DeveloperToolbar.jsm:99:14 116 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:277:19 117 INFO exports.promiseEach/</callNext/onSuccess@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:269:11 118 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 119 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 120 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 121 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9 122 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9 123 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:278:7 124 INFO exports.promiseEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:281:5 125 INFO Promise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/promise.js:48:5 126 INFO exports.promiseEach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:258:1 127 INFO CommandUtils.createButtons@resource:///modules/devtools/DeveloperToolbar.jsm:97:1 128 INFO Toolbox.prototype._buildButtons/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:601:14 129 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 130 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 131 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9 132 INFO Toolbox.prototype._buildButtons@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:599:12 133 INFO Toolbox.prototype.open/</domReady@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:267:30 and 9 INFO console.error: 10 INFO Message: TypeError: window is null 11 INFO Stack: 12 INFO exports.items<.state.isChecked@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/paintflashing.js:106:15 13 INFO CommandUtils.createButtons/</</onChange@resource:///modules/devtools/DeveloperToolbar.jsm:156:27 14 INFO CommandUtils.createButtons/</<@resource:///modules/devtools/DeveloperToolbar.jsm:167:1 15 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 16 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 17 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9 18 INFO CommandUtils.createButtons/<@resource:///modules/devtools/DeveloperToolbar.jsm:99:14 19 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:277:19 20 INFO exports.promiseEach/</callNext/onSuccess@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:269:11 21 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 22 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 23 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 24 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9 25 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9 26 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:278:7 27 INFO exports.promiseEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:281:5 28 INFO Promise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/promise.js:48:5 29 INFO exports.promiseEach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:258:1 30 INFO CommandUtils.createButtons@resource:///modules/devtools/DeveloperToolbar.jsm:97:1 31 INFO Toolbox.prototype._buildButtons/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:601:14 32 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 33 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 34 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11 35 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11 36 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 37 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11 38 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11 39 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 40 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 41 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11 42 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11 43 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40 44 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43 45 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11 46 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11 47 INFO Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23 48 INFO this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

10 more. 88 down, 108 to go

101 down, 95 to go

128 down, 68 to go

Rebased.

146 down, 50 to go

I've done about all I can do. There are about 40 left. Many of them are tough ones. We could knock a bunch out if we figured out how to get around some common patterns though. I had to comment out a bunch that need expectUncaughtException() in regular mode, but fail with that in e10s mode as the errors thrown from content don't reach the test or something like that. A bunch of the unconverted tests are trying to send mouse events to a content button, which doesn't work under e10s. Others I left are just trying to access variables from the content window. Additionally, console.log() from content may not be showing up in the browser console, according to the test browser_console.js.

Rebased. Comments above stand.

(In reply to Heather Arthur [:harth] from comment #18) > Created attachment 8483893 [details] [diff] [review] > webconsole-e10s-WIP16.patch > > I've done about all I can do. There are about 40 left. Many of them are > tough ones. We could knock a bunch out if we figured out how to get around > some common patterns though. > > I had to comment out a bunch that need expectUncaughtException() in regular > mode, but fail with that in e10s mode as the errors thrown from content > don't reach the test or something like that. > > A bunch of the unconverted tests are trying to send mouse events to a > content button, which doesn't work under e10s. > > Others I left are just trying to access variables from the content window. > > Additionally, console.log() from content may not be showing up in the > browser console, according to the test browser_console.js. Great! I'd say it's ready for review if browser.ini is updated to remove the semicolons and instead add skip-if = e10s for the still affected tests.

(In reply to Heather Arthur [:harth] from comment #18) > I've done about all I can do. There are about 40 left. Many of them are > tough ones. We could knock a bunch out if we figured out how to get around > some common patterns though. > > I had to comment out a bunch that need expectUncaughtException() in regular > mode, but fail with that in e10s mode as the errors thrown from content > don't reach the test or something like that. > > A bunch of the unconverted tests are trying to send mouse events to a > content button, which doesn't work under e10s. Heather, are these e10s issues with ours test frameworks? Do you need any guidance from e10s developers on fixing the common problem patterns? > Additionally, console.log() from content may not be showing up in the > browser console, according to the test browser_console.js. I think this is bug 1060093.

(In reply to Chris Peterson (:cpeterson) from comment #21) > (In reply to Heather Arthur [:harth] from comment #18) > > I had to comment out a bunch that need expectUncaughtException() in regular > > mode, but fail with that in e10s mode as the errors thrown from content > > don't reach the test or something like that. > > > > A bunch of the unconverted tests are trying to send mouse events to a > > content button, which doesn't work under e10s. > > Heather, are these e10s issues with ours test frameworks? Do you need any > guidance from e10s developers on fixing the common problem patterns? These issues I think would be common to all tests. expectUncaughtException() is a SimpleTest thing, and the mouse events are sent using EventUtils. the expectUncaughtException() issue would be fine if we completely switched to running e10s only, as we could just take them all out. > > Additionally, console.log() from content may not be showing up in the > > browser console, according to the test browser_console.js. > > I think this is bug 1060093. What's odd is that the logs show up in the content console, but don't show up in the browser console. At least in the test. So I'm not sure it's the same.

Update: I tried converting all the commented-out tests to skip-if, and curiously got a crash in e10s mode when running the lot.

Here's the patch with skip-ifs instead of comments. I just ran them again. Maybe what I was seeing was a tab crash. In one test, a test crashes and the test times out.

(In reply to Heather Arthur [:harth] from comment #24) > Created attachment 8486092 [details] [diff] [review] > webconsole-e10s-WIP18.patch > > Here's the patch with skip-ifs instead of comments. I just ran them again. > Maybe what I was seeing was a tab crash. In one test, a test crashes and the > test times out. Went ahead and pushed to try with e10s enabled to see what happens there: https://tbpl.mozilla.org/?tree=Try&rev=1f368856f6c9

(In reply to Heather Arthur [:harth] from comment #24) > Created attachment 8486092 [details] [diff] [review] > webconsole-e10s-WIP18.patch > > Here's the patch with skip-ifs instead of comments. I just ran them again. > Maybe what I was seeing was a tab crash. In one test, a test crashes and the > test times out. Looks like there is a crash on browser_webconsole_split_persist.js [0] - https://tbpl.mozilla.org/php/getParsedLog.php?id=47641876&full=1&branch=try. I'd suggest just skipping that test with a comment (and bucketing it with the other difficult ones) if it gets the suite running again.

I see a few more errors and leaks on the linux debug build: https://tbpl.mozilla.org/php/getParsedLog.php?id=47640037&full=1&branch=try INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_bug664688_sandbox_update_after_navigation.js | uncaught exception - TypeError: contentWindow is null at chrome://browser/content/browser.js:4509 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_658368_time_methods.js | Test timed out - expected PASS INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_clickable_urls.js | Test timed out - expected PASS Leaks: TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_split_escape_key.js | leaked 2 window(s) until shutdown [url = about:blank] TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_split_focus.js | leaked 2 window(s) until shutdown [url = about:blank] TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_output_table.js | leaked 1 window(s) until shutdown [url = http://example.com/browser/browser/devtools/webconsole/test/test-console-table.html]

I wouldn't worry about the leaks yet, failed tests usually don't clean up after themselves.

Rebased. Now we need to convert the non-e10s tests to loadTab() and openConsole()-as-a-promise. I've started on that.

Sorry, huge patch. This rewrites all the webconsole tests to use new promise-y helper methods (loadTab and openConsole). It also changes some tests to work on e10s. Some are disabled with e10s as they are intermittently failing or would take to long to fix this round. Still getting leaks from various tests on debug builds (https://tbpl.mozilla.org/?tree=Try&rev=65f822e21b1c), does it still stand that that doesn't matter?

Comment on attachment 8502045 [details] [diff] [review] webconsole-e10s-WIP20.patch Review of attachment 8502045 [details] [diff] [review]: ----------------------------------------------------------------- What I meant above was that we should only start working on fixing the leaks after we've made sure that no other test failures occur. This seems to be the case in some of the oranges I looked at in the last try run, but if you prefer to just disable the leaky tests for now, I'm also fine with that. It's always good to make concrete steps forward. Related to that, there are a lot of oranges in unrelated areas, like the style inspector that are obscuring the results of the try run. Didn't we disable all the failing tests/suites or is it that your try push just wasn't rebased on the latest fx-team tip?

As much as I'd like to get this in the bag, I'm going to focus on fixing some e10s functionality in other places. These pass locally for me, but there are several crashes and memory leaks on try. Whenever I disable the leaks, leaks pop up in other tests and whenever I disable a crash, a crash pops up somewhere else. This probably means the crashes are due to running out of memory, I think. Latest run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=934899582a7d

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

Okay, this is it. head.js now defines loadTab() and removes addTab(), as well as makes openConsole() promise-y. A lot of changes are because of that. A lot of tests were also cleaned up to use an asyncTest() helper and yields. Passes plain try: https://tbpl.mozilla.org/?tree=Try&rev=e51bf4cc7904 I've disabled the tests that time out here: https://tbpl.mozilla.org/?tree=Try&rev=da4892fceaac There are still some leaks, but they exist throughout our code and this is bug 1094749. This leaves about 40 tests disabled. 10 of these are due to the expectUncaughtException() calls I mentioned in comment 18, which I'll file a followup for. about 8 are disabled just for Linux debug timeouts.

Comment on attachment 8534511 [details] [diff] [review] webconsole-e10s-tests-v22.patch Review of attachment 8534511 [details] [diff] [review]: ----------------------------------------------------------------- My eyes are bleeding after reviewing all of this, but I'm so excited about the task-based rewrite! W00t! ::: browser/devtools/webconsole/test/browser_warn_user_about_replaced_api.js @@ +22,5 @@ > + let hud2 = yield openConsole(); > + > + yield testWarningPresent(hud2); > + > + Services.prefs.clearUserPref(PREF) Nit: missing semicolon. ::: browser/devtools/webconsole/test/browser_webconsole_autocomplete_in_debugger_stackframe.js @@ +137,5 @@ > openDebugger().then(() => { > gStackframes.selectFrame(1); > > info("openConsole"); > + executeSoon(() => openConsole().then(testDriver.next())); then() should have a lambda that calls next() (not use its return value as a callback): then(() => testDriver.next()) ::: browser/devtools/webconsole/test/browser_webconsole_autocomplete_popup_close_on_tab_switch.js @@ +27,5 @@ > + panel.addEventListener("popupshown", function popupOpened() { > + panel.removeEventListener("popupshown", popupOpened, false); > + loadTab("data:text/html;charset=utf-8,<p>testing autocomplete closes").then(() => { > + finished.resolve(); > + }); Nit: then(() => finished.resolve()) is shorter. ::: browser/devtools/webconsole/test/browser_webconsole_bug_595934_message_categories.js @@ +1,1 @@ > + Hmm? ::: browser/devtools/webconsole/test/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js @@ +1,1 @@ > + Look, there it is again! ::: browser/devtools/webconsole/test/head.js @@ +17,5 @@ > + > +/* > + > + loadTab(TEST_URI).then(() => { > + openConsole().then(() => { Did you mean to add a comment here, like "common test setup code"? @@ +399,4 @@ > } > } > > + setTimeout(wait, 100); Wouldn't executeSoon() be better in this case? It doesn't seem like we actually need the 100 msec delay (although I don't know if we ever succeed or fail in this first call either; if not, a timeout is better). ::: browser/devtools/webconsole/test/test-console-output-events.html @@ +19,5 @@ > document.addEventListener("mousemove", eventLogger); > document.addEventListener("keypress", eventLogger); > + > + synthesizeKeyPress("a", {shiftKey: true}); > + synthesizeMouseMove(); The test expects these events in the reverse order, is this order intentional?

Fixed to comments. Thanks for doing the huge review Panos. Plain try: https://tbpl.mozilla.org/?tree=Try&rev=67337d785dda e10s: https://tbpl.mozilla.org/?tree=Try&rev=56e43e6ed63d (super orange, but they are known or leaks probably due to bug 1073352)

Needs rebasing, sorry :(

Rebased. Try: https://tbpl.mozilla.org/?tree=Try&rev=577b61c32713 e10s: https://tbpl.mozilla.org/?tree=Try&rev=1032157053d9

Leaving the rest of the tests to someone else.

Filed bug 1112599 to track enabling the rest of the web console tests.