1244120 - Re-enable the browser_rules_content_02.js with e10s

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect)

Description

[Patrick Brosset <:pbro>]

This test uses the right-click browser contextual menu to test that the rule-view initializes correctly when the toolbox gets opened from there.

This used to not work with e10s at some stage, which is when the test was disabled.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1244120 - Enable browser_rules_content_02.js with e10s; r=bgrins

Extracted a shared helper to open the browser context menu and choose
the 'inspect element' item. This helper works with e10s.
Adapted it a little bit so it waits for the right events in order to
make sure the inspector is ready.

Used this in browser_inspector_initialization.js and
browser_rules_content_02.js.

Also removed a now useless inspector-updated event that was trigger from
the animation-inspector panel in some situation. This was left behind
from a long time ago but didn't serve any purpose anymore.

Review commit: https://reviewboard.mozilla.org/r/32789/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32789/

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8713625 [details]
MozReview Request: Bug 1244120 - Enable browser_rules_content_02.js with e10s; r=bgrins

https://reviewboard.mozilla.org/r/32789/#review29661

::: devtools/client/inspector/test/head.js:166
(Diff revision 1)
> + * @param {Boolean} expectNewToolbox By default, this helper assumes that the

Curious if we need this parameter or if we could instead do something like:

let target = TargetFactory.forTab(gBrowser.selectedTab);
let expectNewToolbox = !!gDevTools.getToolbox(target)

Comment 3

[Patrick Brosset <:pbro>]

Try showed test failures with non-e10s, so I investigated more and ended up fixing inspectNode in nsContextMenu.js. It returned a promise already, but that promise wasn't waiting for the node to be selected and the inspector to be ready.
With this fixed, it's now easy for tests, whether they run with or without e10s, to execute this menu item's action and be sure that the inspector is ready.
I'll upload a new patch soon and will as a browser peer to review the changes in nsContextMenu.js.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug_1244120_-_Enable_browser_rules_content_02.js_w.diff

Gijs, do you mind taking a look at the changes I made to nsContextMenu.js in this patch?

Comment 5

[:Gijs (he/him)]

Comment on attachment 8715291 [details] [diff] [review]
Bug_1244120_-_Enable_browser_rules_content_02.js_w.diff

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

::: browser/base/content/nsContextMenu.js
@@ +558,5 @@
>    openPasswordManager: function() {
>      LoginHelper.openPasswordManager(window, gContextMenuContentData.documentURIObject.host);
>    },
>  
> +  inspectNode: function() {

Nit: inspectNode() {

@@ +568,5 @@
>        let inspector = toolbox.getCurrentPanel();
> +
> +      // new-node-front tells us when the node has been selected, whether the
> +      // browser is remote or not.
> +      let onNewNode = inspector.selection.once("new-node-front");

I don't know enough about devtools to feel comfortable reviewing this. Two things I can already note:

1) are we guaranteed to get the new-node-front message in a later event tick from the showToolbox promise being resolved? That is, is there a guarantee that that message/event won't precede the showToolbox being resolved? Same question for the inspector-updated event.

2) While I don't want to make you fix our sidebar/social-panel context menu in this bug, I'd like to make sure this patch doesn't make things worse when the inspected node isn't in the selected tab's document. What happens in that case?

Comment 6

[Patrick Brosset <:pbro>]

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8715291 [details] [diff] [review]
> Bug_1244120_-_Enable_browser_rules_content_02.js_w.diff
> 
> Review of attachment 8715291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/nsContextMenu.js
> @@ +558,5 @@
> >    openPasswordManager: function() {
> >      LoginHelper.openPasswordManager(window, gContextMenuContentData.documentURIObject.host);
> >    },
> >  
> > +  inspectNode: function() {
> 
> Nit: inspectNode() {
Fine with me, but none of the other methods in this file seem to follow this convention.
So adding this would result in the nsContextMenu.prototype containing 3 different types of function properties:

name: function CM_name() {}
name: function() {}
name() {}

> @@ +568,5 @@
> >        let inspector = toolbox.getCurrentPanel();
> > +
> > +      // new-node-front tells us when the node has been selected, whether the
> > +      // browser is remote or not.
> > +      let onNewNode = inspector.selection.once("new-node-front");
> 
> I don't know enough about devtools to feel comfortable reviewing this. Two
> things I can already note:
> 
> 1) are we guaranteed to get the new-node-front message in a later event tick
> from the showToolbox promise being resolved? That is, is there a guarantee
> that that message/event won't precede the showToolbox being resolved?
So, showToolbox does a lot of things like opening the toolbox, loading the inspector iframe, selecting the default node (<body> or the last selected node), etc... and finally returning a promise that resolves when the panel is ready to be used, but we don't really care because later in inspectNode we force a new selection (with inspector.selection.setNodeFront). So I'm only listening to new-node-front to know when this new selection is done.
So, yes, there are new-node-front events preceding the showToolbox promise being resolved, but those are not the ones I'm interested in.
> Same question for the inspector-updated event.
Yes, inspector-updated is guaranteed to be emitted after new-node-front is emitted. That's because the inspector asynchronously updates all sidebar panels.
In fact, I think I could do without new-node-front completely, and just listen to inspector-updated.
> 2) While I don't want to make you fix our sidebar/social-panel context menu
> in this bug, I'd like to make sure this patch doesn't make things worse when
> the inspected node isn't in the selected tab's document. What happens in
> that case?
After a quick test, it doesn't make things worse. They're still broken in the same way (see bug 1226079).

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8715291 [details] [diff] [review]
Bug_1244120_-_Enable_browser_rules_content_02.js_w.diff

As per Gijs' comment, transferring this review to a devtools reviewer.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8715291 [details] [diff] [review]
Bug_1244120_-_Enable_browser_rules_content_02.js_w.diff

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

::: browser/base/content/nsContextMenu.js
@@ +582,5 @@
> +
> +      return onNewNode.then(() => {
> +        // Now that the node has been selected, wait until the inspector is
> +        // fully updated.
> +        return inspector.once("inspector-updated");

I agree that inspector-updated is the only necessary event and we should be able to drop the new-node-front part

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> I agree that inspector-updated is the only necessary event and we should be
> able to drop the new-node-front part
I spent more time trying to do this, but didn't manage. The init sequence of the inspector is kind of weird. With e10s, we essentially receive 2 inspector-updated events (one when the default node is selected and one when the new selection is made). So if I wanted to only use that event, I'd have to listen to it twice.
Listening to new-node-front in the code makes the intent clearer and there's no need to have some special e10s logic. So I'm going to keep it this way for now.

Comment 10

[Patrick Brosset <:pbro>]

My last try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5737fc90aa&group_state=expanded shows some unrelated failures. I couldn't reproduce any of these locally and the logs don't mention anything that seems related to my patch. So I'm going to ignore those.

Comment 11

[Pulsebot]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for frequent leaks on Win7: https://treeherder.mozilla.org/logviewer.html#?job_id=7059101&repo=fx-team

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

Comment 13

[Patrick Brosset <:pbro>]

I can't reproduce this leak locally unfortunately.
It happens on e10s windows 7 debug builds, so I can, at least, reproduce on TRY by triggering new builds.

devtools/client/inspector/rules/test/browser_rules_content_02.js | leaked 1 docShell(s) until shutdown

I'm going to try various fix attempts and test them out on TRY.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: Bug_1244120_-_Enable_browser_rules_content_02.js_w.diff

New, rebased, patch which also uses the new clickOnInspectMenuItem helper in browser_markup_keybindings_04.js.
Carrying R+ over.
Still leaking, but investigation is ongoing, with help from jdescottes.

Comment 15

[Patrick Brosset <:pbro>]

As discussed with Julian, the fix for the leak is going to be more time consuming than expected.
Julian filed the follow-up bug 1250058 to work on it. In the meantime I'll land the changes here with the test skipped with win&e10s&debug.

Comment 16

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=364d40083490&group_state=expanded

Comment 17

[Patrick Brosset <:pbro>]

Attachment: Bug_1244120_-_Enable_browser_rules_content_02.js_w.diff

Here's the patch with the test disabled on win&debug&e10s to match the try push in comment 16.

Comment 18

[Pulsebot]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1bd2197b4d48