1112599 - [meta] Enable remaining devtools/webconsole tests with e10s

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 1042253 enabled most of the web console tests with e10s, but a few still remain disabled:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser.ini

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65cc1b187738

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Enable all remaining webconsole tests in e10s

Tentative patch.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

All tests pass locally on linux64 opt.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #3)
> All tests pass locally on linux64 opt.

Scratch that, I didn't use --e10s.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Part 1 v2

This version is less ambitious, but passes in both opt and debug modes in e10s locally.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Let's see if try has recovered:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56895c5184f8

Comment 7

[Pulsebot]

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

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Try is hopeless, but this works well on my system. Let's see what happens.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing frequent Linux32 browser_webconsole_trackingprotection_errors.js shutdown hangs.
https://treeherder.mozilla.org/logviewer.html#?job_id=4348500&repo=fx-team

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

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Weird, this patch didn't actually touch browser_webconsole_trackingprotection_errors.js. It did enable the one before that in browser.ini, but in the above mentioned log that didn't run immediately before browser_webconsole_trackingprotection_errors.js either.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Enable expectUncaughtException tests v1

While working on bug 1184172, I realized many webconsole tests
are working correctly on e10s. We just have to prevent calling exceptUncaughtException.
The exception are always thrown in the child process and the test harness
doesn't care about exception happenning in child processes.

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

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8652369 [details] [diff] [review]
Enable expectUncaughtException tests v1

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

::: browser/devtools/webconsole/test/browser.ini
@@ +232,5 @@
>  [browser_webconsole_bug_595223_file_uri.js]
>  [browser_webconsole_bug_595350_multiple_windows_and_tabs.js]
>  skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s
>  [browser_webconsole_bug_595934_message_categories.js]
> +skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s

Why isn't this removed? You modified the test as far as I can see.

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #12)

> ::: browser/devtools/webconsole/test/browser.ini
> >  [browser_webconsole_bug_595934_message_categories.js]
> > +skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s
> 
> Why isn't this removed? You modified the test as far as I can see.

This test still fails even with this fix, so there is some more work to enable it.

Comment 14

[Pulsebot]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11676835290d

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #10)
> Weird, this patch didn't actually touch
> browser_webconsole_trackingprotection_errors.js. It did enable the one
> before that in browser.ini, but in the above mentioned log that didn't run
> immediately before browser_webconsole_trackingprotection_errors.js either.

The same patch in a new try run causes intermittent shutdown hangs in browser_webconsole_view_source.js, which immediately follows browser_webconsole_trackingprotection_errors.js:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b26a11dcb39

Neither test is enabled (or otherwise directly affected) by this patch.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Some of the try runs I did lately to test some theories:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b26a11dcb39
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e458f7c96ad0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4990406ed58b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e178ea4d5e66
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53ec41da571

I think we have to make the harness wait a bit longer in some of these tests. linux32 with e10s is very slow.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

I'm deleting 2 of the disabled tests in bug 861335.

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: webconsole-e10s-remaining-WIP.patch

stashing a WIP that converts a bunch of the tests

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: webconsole-e10s-more.patch

Lin, this is the state of my current work.  Can you take a look when you get a chance?  Here's a try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=178dd3a0ca1f.

Also, tests that aren't part of this patch are fair game to work on.  Maybe we should start moving each test into it's own bug to make it easier to track though.

Comment 21

[Lin Clark [:linclark]]

Comment on attachment 8713028 [details] [diff] [review]
webconsole-e10s-more.patch

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

I'll be finishing the review later today, but just wanted to get these comments up for discussion.

::: devtools/client/webconsole/test/browser.ini
@@ -162,5 @@
>  [browser_console_dead_objects.js]
>  skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s
>  [browser_console_copy_entire_message_context_menu.js]
>  [browser_console_error_source_click.js]
> -skip-if = buildapp == 'mulet' || e10s # Bug 1042253 - webconsole e10s tests

I don't know what "mulet" is. Should we leave the skip statement for "mulet"?

@@ -167,3 @@
>  [browser_console_filters.js]
>  [browser_console_iframe_messages.js]
> -skip-if = buildapp == 'mulet' || e10s # Bug 1042253 - webconsole e10s tests

Ditto.

::: devtools/client/webconsole/test/browser_console_error_source_click.js
@@ +24,5 @@
>      ok(hud, "browser console opened");
>  
> +    // On e10s, the exception is triggered in child process
> +    // and is ignored by test harness
> +    if (!Services.appinfo.browserTabsRemoteAutostart) {

From looking around in the code base, it looks like Services.appinfo.browserTabsRemoteAutostart means "e10s is enabled". That isn't obvious to me from the property name. That is a problem outside the scope of this issue. But in this issue, maybe it's worth adding a helper to head.js which makes it explicit (like e10sIsEnabled())? Or mentioning in the comment?

::: devtools/client/webconsole/test/browser_console_iframe_messages.js
@@ +70,5 @@
> +  yield closeConsole();
> +  info("web console closed");
> +
> +  // The browser console doesn't show page's console.log statements in
> +  // e10s windows. See Bug 1241289.

Nit. Should this be a TODO that is dependent on Bug 1241289?

@@ +79,5 @@
> +  }
> +});
> +
> +function* testWebConsole(hud) {
> +  ok(hud, "web console opened");

Nit. Should ok statements live next to their corresponding yield? e.g. let hud = yield openConsole();

Comment 22

[Lin Clark [:linclark]]

I had a convo with Alex about mulet and have created this bug to address it: Bug 1243777.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Here's a try push
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=178dd3a0ca1f.

Grr looks like this is perma-oranging browser_perf-overview-render-02.js somehow.. Maybe Bug 1240509?  Or Bug 1204174

Comment 24

[Lin Clark [:linclark]]

Comment on attachment 8713028 [details] [diff] [review]
webconsole-e10s-more.patch

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

Here are a couple more questions. I'm setting back to needs work due to the perma-orange that you pointed out.

::: devtools/client/webconsole/test/browser_console_variables_view_while_debugging.js
@@ +25,5 @@
> +
> +function* waitForFrameAdded(dbgPanel) {
> +  let thread = dbgPanel.panelWin.DebuggerController.activeThread;
> +
> +  info("Waiting for onFramesAdded");

Nit. onFramesAdded is the name of a function that's being removed. Maybe change to "Waiting for framesadded"?

::: devtools/client/webconsole/test/browser_console_variables_view_while_debugging_and_inspecting.js
@@ +26,5 @@
> +
> +function* waitForFrameAdded(dbgPanel) {
> +  let thread = dbgPanel.panelWin.DebuggerController.activeThread;
> +
> +  info("Waiting for onFramesAdded");

See above.

::: devtools/client/webconsole/test/browser_webconsole_bug_588342_document_focus.js
@@ +17,3 @@
>  
> +  // This is skipped in e10s due to Bug 1241707 not properly restoring
> +  // focus.

Nit. Should this be a TODO?

::: devtools/client/webconsole/test/browser_webconsole_bug_593003_iframe_wrong_hud.js
@@ +20,3 @@
>  
> +  let tab1 = (yield loadTab(TEST_URI)).tab;
> +  content.console.log("FOO");

Should this be in a ContentTask.spawn call?

@@ +32,5 @@
> +  yield checkMessages(tab1, tab2);
> +
> +  info("Cleaning up");
> +  yield closeConsole(tab1);
> +  yield closeConsole(tab2);

Should this go in a registerCleanupFunction?

Comment 25

[Lin Clark [:linclark]]

I've added issues for all of the tests in https://docs.google.com/spreadsheets/d/10UeyRoiWV2HjkWwAU51HXyXAV7YLi4BjDm55mr5Xv6c/edit#gid=1777180571

Here are the numbers for the tests in the WIP patch. I'm guessing moving them each to their own bug would have the extra advantage of making it easier to land the ones that don't cause perma-orange? But if we don't want to split up this patch, we could close these ones out.

Bug 1243963 - browser_console_error_source_click.js
Bug 1243983 - browser_console_iframe_messages.js
Bug 1243995 - browser_console_optimized_out_vars.js
Bug 1243977 - browser_console_variables_view.js
Bug 1243968 - browser_console_variables_view_while_debugging.js 
Bug 1243962 - browser_console_variables_view_while_debugging_and_inspecting.js 
Bug 1243959 - browser_webconsole_autocomplete_in_debugger_stackframe.js
Bug 1243992 - browser_webconsole_bug_585991_autocomplete_keys.js
Bug 1243958 - browser_webconsole_bug_588342_document_focus.js
Bug 1243984 - browser_webconsole_bug_593003_iframe_wrong_hud.js
Bug 1243970 - browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js

Comment 26

[Brian Grinstead [:bgrins]]

New try push that looks better (still a lot of perf errors but not perma-oranging them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98c182a1c579&selectedJob=16073959

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark [:linclark] from comment #21)
> ::: devtools/client/webconsole/test/browser_console_error_source_click.js
> @@ +24,5 @@
> >      ok(hud, "browser console opened");
> >  
> > +    // On e10s, the exception is triggered in child process
> > +    // and is ignored by test harness
> > +    if (!Services.appinfo.browserTabsRemoteAutostart) {
> 
> From looking around in the code base, it looks like
> Services.appinfo.browserTabsRemoteAutostart means "e10s is enabled". That
> isn't obvious to me from the property name. That is a problem outside the
> scope of this issue. But in this issue, maybe it's worth adding a helper to
> head.js which makes it explicit (like e10sIsEnabled())? Or mentioning in the
> comment?

Probably makes sense as a constant in shared-head (or to revisit and see if there's a better named constant available now on Services).  Let's move that into a follow up since these calls are all over our tests (not just in webconsole).

> ::: devtools/client/webconsole/test/browser_console_iframe_messages.js
> @@ +70,5 @@
> > +  yield closeConsole();
> > +  info("web console closed");
> > +
> > +  // The browser console doesn't show page's console.log statements in
> > +  // e10s windows. See Bug 1241289.
> 
> Nit. Should this be a TODO that is dependent on Bug 1241289?

Yes thanks, I've updated that.

> @@ +79,5 @@
> > +  }
> > +});
> > +
> > +function* testWebConsole(hud) {
> > +  ok(hud, "web console opened");
> 
> Nit. Should ok statements live next to their corresponding yield? e.g. let
> hud = yield openConsole();

Moved it

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8713028 [details] [diff] [review]
webconsole-e10s-more.patch

Moved this mega patch into the bugs in comment 25

Comment 29

[Brian Grinstead [:bgrins]]

Converting this to a meta bug

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

All blockers are fixed