Re-enable the browser_rules_content_02.js with e10s

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

(Blocks 1 bug)

unspecified
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
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/
Attachment #8713625 - Flags: review?(bgrinstead)
Assignee

Updated

3 years ago
Assignee: nobody → pbrosset
Assignee

Updated

3 years ago
Status: NEW → ASSIGNED
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)
Attachment #8713625 - Flags: review?(bgrinstead) → review+
Blocks: e10s-tests
tracking-e10s: --- → +
Assignee

Comment 3

3 years ago
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.
Assignee

Comment 4

3 years ago
Gijs, do you mind taking a look at the changes I made to nsContextMenu.js in this patch?
Attachment #8713625 - Attachment is obsolete: true
Attachment #8715291 - Flags: review?(gijskruitbosch+bugs)

Comment 5

3 years ago
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?
Attachment #8715291 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 6

3 years ago
(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).
Assignee

Comment 7

3 years ago
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.
Attachment #8715291 - Flags: review?(bgrinstead)
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
Attachment #8715291 - Flags: review?(bgrinstead) → review+
Assignee

Comment 9

3 years ago
(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.
Assignee

Comment 10

3 years ago
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.
Assignee

Comment 13

3 years ago
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.
Flags: needinfo?(pbrosset)
Assignee

Updated

3 years ago
Blocks: 1246677
Assignee

Comment 14

3 years ago
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.
Attachment #8715291 - Attachment is obsolete: true
Attachment #8721213 - Flags: review+
Assignee

Comment 15

3 years ago
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.
Assignee

Comment 17

3 years ago
Here's the patch with the test disabled on win&debug&e10s to match the try push in comment 16.
Attachment #8721213 - Attachment is obsolete: true
Attachment #8721946 - Flags: review+

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1bd2197b4d48
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.