Closed Bug 1297362 Opened 3 years ago Closed 3 years ago

Eliminate CPOWs from Netmonitor mochitests

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jsnajdr, Assigned: jsnajdr)

References

Details

Attachments

(7 files)

There's a huge number of Netmonitor intermittents caused by "dead CPOW" access. This is caused by calling content code through tab.linkedBrowser.contentWindow.wrappedJSObject, exported as "debuggee" by initNetMonitor at [1].

Let's convert all these usages to ContentTasks.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#134
Assignee: nobody → jsnajdr
The CPOW failures usually happen at [1], when the "debuggee" CPOW is retrieved and returned from the initNetMonitor function.

This means it's not possible to fix the tests one by one. Instead, the plan is:

1. Rewrite all tests to use ContentTask instead of accessing the "debuggee".
2. After there are no tests that need it, stop returning "debuggee" from initNetMonitor.

Most of the changes will happen in part 1, but the CPOWs will be eliminated only in part 2, all at once.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#134
Honza, Alex, please have a look at the two attached patches and tell me if they're feasible to review.

This is what I'm doing:
- replace all uses of "debuggee" CPOW with a ContentTask
- the ContentTask returns a promise I'm waiting for, so it's convenient to rewrite stuff to Task.js on the way
- I'm also making the test files eslint-clean at the same time

Is it too much change bundled together? I think that MozReview does a great job showing the changes in a very friendly manner, but I'm not sure.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(odvarko)
Comment on attachment 8784266 [details]
Bug 1297362 - Part 2: Eliminate CPOWs from Netmonitor mochitests C

https://reviewboard.mozilla.org/r/73794/#review71672

Thanks again jarda!

Ideally it would have been easier to review in three patches: one for eslint, one for task and one for CPOW,
but that's ok. The patch wasn't that big, only a couple of tests were seriously modified and required additional attention.

r+ assuming try is happy with all that. But I assume you will fixes random intermittents by always waiting for network events first.

::: devtools/client/netmonitor/test/browser_net_cached-status.js
(Diff revision 1)
> -// /////////////////
> +"use strict";
> -//
> -// Whitelisting this test.
> -// As part of bug 1077403, the leaking uncaught rejection should be fixed.
> -//
> -thisTestLeaksUncaughtRejectionsAndShouldBeFixed("TypeError: can't convert undefined to object");

You may close bug 1077403 if that's no longer rejecting a promise.

::: devtools/client/netmonitor/test/browser_net_cause.js:20
(Diff revision 1)
>      method: "GET",
>      url: CAUSE_URL,
>      causeType: "document",
>      causeUri: "",
> -    // The document load is from JS function in e10s, native in non-e10s
> -    stack: !gMultiProcessBrowser
> +    // The document load has internal privileged JS code on the stack
> +    stack: true

Why this change? do we only run in e10s now?

::: devtools/client/netmonitor/test/browser_net_cause.js:87
(Diff revision 1)
>    // page is loaded. That's why we first load a bogus page from SIMPLE_URL,
>    // and only then load the real thing from CAUSE_URL - we want to catch
>    // all the requests the page is making, not only the XHRs.
>    // We can't use about:blank here, because initNetMonitor checks that the
>    // page has actually made at least one request.
> -  let [, debuggee, monitor] = yield initNetMonitor(SIMPLE_URL);
> +  let [tab, , monitor] = yield initNetMonitor(SIMPLE_URL);

I imagine a good followup would be to stop returning debuggee from initNetMonitor?

::: devtools/client/netmonitor/test/browser_net_cause.js:95
(Diff revision 1)
>    RequestsMenu.lazyUpdate = false;
>  
> -  debuggee.location = CAUSE_URL;
> -
> -  yield waitForNetworkEvents(monitor, EXPECTED_REQUESTS.length);
> +  let wait = waitForNetworkEvents(monitor, EXPECTED_REQUESTS.length);
> +  yield ContentTask.spawn(tab.linkedBrowser, CAUSE_URL, function* (url) {
> +    content.location = url;
> +  });

Some prefer: tab.linkedBrowser.loadURI(CAUSE_URL)
I don't have a strong opinion on this.

::: devtools/client/netmonitor/test/browser_net_clear.js:64
(Diff revision 1)
> -    });
> -
> -    aDebuggee.location.reload();
> +   * Tell the content to reload
> +   */
> +  function reload() {
> +    return ContentTask.spawn(tab.linkedBrowser, {}, function* () {
> +      content.location.reload();
> -  });
> +    });

Similar comment than loadURI, you could do: tab.linkedBrowser.reload()

::: devtools/client/netmonitor/test/browser_net_copy_headers.js:22
(Diff revision 1)
>    RequestsMenu.lazyUpdate = false;
>  
> -  aDebuggee.location.reload();
> -  yield waitForNetworkEvents(aMonitor, 1);
> +  let wait = waitForNetworkEvents(monitor, 1);
> +  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {
> +    content.location.reload();
> +  });

Same, tab.linkedBrowser.reload is an alternative.

::: devtools/client/netmonitor/test/browser_net_copy_params.js:88
(Diff revision 1)
> -        aHidden, "The \"Copy POST Data\" context menu item should" + (aHidden ? " " : " not ") + "be hidden.");
> +      "The \"Copy POST Data\" context menu item should" + (hidden ? " " : " not ") +
> +        "be hidden.");
> -    }
> +  }
>  
> -    function testCopyPostData(aPostData) {
> -      return RequestsMenu.copyPostData().then(() => {
> +  function* testCopyPostData(postData) {
> +    yield RequestsMenu.copyPostData();

I don't know much about clipboard, but shouldn't you also use waitForClipboardPromise over here?

::: devtools/client/netmonitor/test/browser_net_cyrillic-02.js:23
(Diff revision 1)
> -    RequestsMenu.lazyUpdate = false;
> +  RequestsMenu.lazyUpdate = false;
>  
> -    waitForNetworkEvents(aMonitor, 1).then(() => {
> +  let wait = waitForNetworkEvents(monitor, 1);
> +  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {
> +    content.location.reload();
> +  });

tab.linkedBrowser.reload() ?
Attachment #8784266 - Flags: review?(poirot.alex) → review+
Flags: needinfo?(poirot.alex)
Comment on attachment 8784263 [details]
Bug 1297362 - Part 1: Eliminate CPOWs from Netmonitor mochitests A

https://reviewboard.mozilla.org/r/73792/#review71674

Looks good to me as well, assuming try is also happy with this part.

::: devtools/client/netmonitor/test/browser_net_autoscroll.js
(Diff revision 1)
> -// /////////////////
> +"use strict";
> -//
> -// Whitelisting this test.
> -// As part of bug 1077403, the leaking uncaught rejection should be fixed.
> -//
> -thisTestLeaksUncaughtRejectionsAndShouldBeFixed("TypeError: aValue.content is undefined");

Ensure running on all platforms, especially the slow ones like linux32 to confirm this rejection is gone.
You may drop a comment in 1077403 to say that all/most of the one from netmonitor are gone.
Attachment #8784263 - Flags: review+
Comment on attachment 8784263 [details]
Bug 1297362 - Part 1: Eliminate CPOWs from Netmonitor mochitests A

https://reviewboard.mozilla.org/r/73792/#review71680

Nice!

Just like Alex mentioned, eslint changes could be in separate patch (but, it's ok if there is not so many eslint related changes)


All tests, are passing for me localy, but I am seeing 

"A coding exception was thrown in a Promise resolution callback."
"TypeError: win is null"

Is that related to bug 1077403?

Honza

::: devtools/client/netmonitor/test/browser_net_accessibility-01.js:61
(Diff revision 1)
> -      check(1, true);
> +  check(1, true);
> -      RequestsMenu.focusItemAtDelta(-10);
> +  RequestsMenu.focusItemAtDelta(-10);
> -      check(0, true);
> +  check(0, true);
>  
> -      aDebuggee.performRequests(18);
> -      return waitForNetworkEvents(aMonitor, 18);
> +  wait = waitForNetworkEvents(monitor, 18);
> +  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {

You migh want to utilize (at least in some cases) a similar helper like the one defined in devtools\client\scratchpad\test\head

/**
 * A simple wrapper for ContentTask.spawn for more compact code.
 */
function inContent(generator) {
  return ContentTask.spawn(gBrowser.selectedBrowser, {}, generator);
}
Attachment #8784263 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> Just like Alex mentioned, eslint changes could be in separate patch (but,
> it's ok if there is not so many eslint related changes)

Doing that would much harder for me, I'm afraid - having to visit each test twice.

> All tests, are passing for me localy, but I am seeing 
> 
> "A coding exception was thrown in a Promise resolution callback."
> "TypeError: win is null"
> 
> Is that related to bug 1077403?

Which test is that?

Bug 1077403 was reported in October 2014, so I suspect the reasons for ignoring the uncaught rejections are long gone. I plan to test this thoroughly on try before finally landing.

> You migh want to utilize (at least in some cases) a similar helper like the
> one defined in devtools\client\scratchpad\test\head

That's a good idea, I'm not happy about the ContentTask verbosity, either.
(In reply to Jarda Snajdr [:jsnajdr] from comment #9)
> (In reply to Jan Honza Odvarko [:Honza] from comment #8)
> > Just like Alex mentioned, eslint changes could be in separate patch (but,
> > it's ok if there is not so many eslint related changes)
> 
> Doing that would much harder for me, I'm afraid - having to visit each test
> twice.
Yeah, I understand, no problem.

> 
> > All tests, are passing for me localy, but I am seeing 
> > 
> > "A coding exception was thrown in a Promise resolution callback."
> > "TypeError: win is null"
> > 
> > Is that related to bug 1077403?
> 
> Which test is that?
I tried devtools/client/netmonitor/test/browser_net_autoscroll.js, but I think others have the same problem (which might indicate that it's unrelated to your changes, still worth to know about it I think).

Honza
Flags: needinfo?(odvarko)
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> > -thisTestLeaksUncaughtRejectionsAndShouldBeFixed("TypeError: can't convert undefined to object");
> 
> You may close bug 1077403 if that's no longer rejecting a promise.

I get no uncaught rejections locally and we'll see on try. Bug 1077403 is almost 2 years old and I suspect the errors have disappeared since then.

> > -    // The document load is from JS function in e10s, native in non-e10s
> > -    stack: !gMultiProcessBrowser
> > +    // The document load has internal privileged JS code on the stack
> > +    stack: true
> 
> Why this change? do we only run in e10s now?

When the document was reloaded over a CPOW, the document load had no JS on its stack. Now, when the reload is done with a ContentTask, there are a few JS frames from the content-side implementation of ContentTask. I guess I'll have to undo this change once I switch to tab.linkedBrowser.reload() - then it'll be an IPDL call with no JS again.

> I imagine a good followup would be to stop returning debuggee from
> initNetMonitor?

Yes, that's the final step and the reason I'm doing all these changes. The intermittent failures will disappear only after initNetMonitor will stop retrieving the debuggee.

> Some prefer: tab.linkedBrowser.loadURI(CAUSE_URL)
> I don't have a strong opinion on this.

Cool, I'll switch to this to avoid ContentTask verbosity.

> I don't know much about clipboard, but shouldn't you also use
> waitForClipboardPromise over here?

Everything is happening synchronously here, but I agree that using waitForClipboardPromise consistently is a good idea. It does additional things like seeding the clipboard value with nonsense before calling the "setup" code.
(In reply to Jan Honza Odvarko [:Honza] from comment #10)
> I tried devtools/client/netmonitor/test/browser_net_autoscroll.js, but I
> think others have the same problem (which might indicate that it's unrelated
> to your changes, still worth to know about it I think).

I can't reproduce this particular error, but I noticed in the past that TypeErrors and ReferenceErrors are sometimes reported, but don't cause the test to fail, because they are caught and usually passed to console.error.

I pushed a complete set of patches for review. I'm going to push several try runs now to make sure everything's OK, so changes are quite likely and you don't need to hurry with the review.

I gave up on reducing the verbosity of ContentTask.spawn's for now - I think it deserves a separate bug. To unify the various ways how the content is told to make requests. Sometimes it's "performRequests" function with no arguments, sometimes there are arguments (count and URL), sometimes performRequestsInContent is used.
Comment on attachment 8785301 [details]
Bug 1297362 - Part 3: Eliminate CPOWs from Netmonitor mochitests D-J

https://reviewboard.mozilla.org/r/74554/#review72736

::: devtools/client/netmonitor/test/browser_net_json-long.js:45
(Diff revision 1)
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.getElementById("details-pane-toggle"));
> +    document.getElementById("details-pane-toggle"));
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[3]);
> +    document.querySelectorAll("#details-pane tab")[3]);
>  
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);

It would be safer to keep listening for RESPONSE_BODY_DISPLAYED *before* we send the mouse events while still yielding it right here.

::: devtools/client/netmonitor/test/browser_net_json-malformed.js:38
(Diff revision 1)
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.getElementById("details-pane-toggle"));
> +    document.getElementById("details-pane-toggle"));
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[3]);
> +    document.querySelectorAll("#details-pane tab")[3]);
>  
> -      let tab = document.querySelectorAll("#details-pane tab")[3];
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);

Same here. The test may work as-is today, but this is a typical race.

::: devtools/client/netmonitor/test/browser_net_json_custom_mime.js:40
(Diff revision 1)
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.getElementById("details-pane-toggle"));
> +    document.getElementById("details-pane-toggle"));
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[3]);
> +    document.querySelectorAll("#details-pane tab")[3]);
>  
> -      let RESPONSE_BODY_DISPLAYED = aMonitor.panelWin.EVENTS.RESPONSE_BODY_DISPLAYED;
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);

Same.

::: devtools/client/netmonitor/test/browser_net_json_text_mime.js:40
(Diff revision 1)
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.getElementById("details-pane-toggle"));
> +    document.getElementById("details-pane-toggle"));
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[3]);
> +    document.querySelectorAll("#details-pane tab")[3]);
>  
> -      let RESPONSE_BODY_DISPLAYED = aMonitor.panelWin.EVENTS.RESPONSE_BODY_DISPLAYED;
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);

Same.

::: devtools/client/netmonitor/test/browser_net_jsonp.js:50
(Diff revision 1)
> -        EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -          document.getElementById("details-pane-toggle"));
> +    document.getElementById("details-pane-toggle"));
> -        EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -          document.querySelectorAll("#details-pane tab")[3]);
> +    document.querySelectorAll("#details-pane tab")[3]);
>  
> -        yield waitFor(aMonitor.panelWin, RESPONSE_BODY_DISPLAYED);
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);

Same.

::: devtools/client/netmonitor/test/browser_net_jsonp.js:55
(Diff revision 1)
> -        yield waitFor(aMonitor.panelWin, RESPONSE_BODY_DISPLAYED);
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);
> -        testResponseTab("$_0123Fun", "\"Hello JSONP!\"");
> +  testResponseTab("$_0123Fun", "\"Hello JSONP!\"");
>  
> -        RequestsMenu.selectedIndex = 1;
> +  RequestsMenu.selectedIndex = 1;
>  
> -        yield waitFor(aMonitor.panelWin, RESPONSE_BODY_DISPLAYED);
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);

Same. Here I imagine it is the selectedIndex = 1 which triggers an action?
Attachment #8785301 - Flags: review?(poirot.alex) → review+
Comment on attachment 8785302 [details]
Bug 1297362 - Part 4: Eliminate CPOWs from Netmonitor mochitests L-P

https://reviewboard.mozilla.org/r/74556/#review72740

::: devtools/client/netmonitor/test/browser_net_large-response.js:42
(Diff revision 1)
> -        document.getElementById("details-pane-toggle"));
> +    document.getElementById("details-pane-toggle"));
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[3]);
> +    document.querySelectorAll("#details-pane tab")[3]);
> -    });
>  
> -    aDebuggee.performRequests(1, CONTENT_TYPE_SJS + "?fmt=html-long");
> +  yield monitor.panelWin.once(EVENTS.RESPONSE_BODY_DISPLAYED);

Again. We should always wait for the event first, before trigerring the action which should send the event.

::: devtools/client/netmonitor/test/browser_net_page-nav.js:30
(Diff revision 1)
>  
> -        aMonitor.panelWin.once(aMonitor.panelWin.EVENTS.TARGET_DID_NAVIGATE, () => {
> -          is(aDebuggee.location, NAVIGATE_URL,
> +    tab.linkedBrowser.loadURI(NAVIGATE_URL);
> +    yield onWillNav;
> -            "Target finished navigating to the correct location.");
>  
> -          aCallback();
> +    is(tab.linkedBrowser.contentWindow.location, SIMPLE_URL,

Note that this is a possible CPOW. (linkedBrowser.contentWindow)

::: devtools/client/netmonitor/test/browser_net_page-nav.js:34
(Diff revision 1)
> -          aCallback();
> -        });
> +    is(tab.linkedBrowser.contentWindow.location, SIMPLE_URL,
> +      "Target started navigating to the correct location.");
> -      });
>  
> -      aDebuggee.location = NAVIGATE_URL;
> +    yield onDidNav;
> +    is(tab.linkedBrowser.contentWindow.location, NAVIGATE_URL,

Same here about CPOW.

::: devtools/client/netmonitor/test/browser_net_page-nav.js:47
(Diff revision 1)
>  
> -        aMonitor.panelWin.once(aMonitor.panelWin.EVENTS.TARGET_DID_NAVIGATE, () => {
> -          is(aDebuggee.location, SIMPLE_URL,
> +    tab.linkedBrowser.loadURI(SIMPLE_URL);
> +    yield onWillNav;
> -            "Target finished navigating back to the previous location.");
>  
> -          aCallback();
> +    is(tab.linkedBrowser.contentWindow.location, NAVIGATE_URL,

CPOW?

::: devtools/client/netmonitor/test/browser_net_page-nav.js:51
(Diff revision 1)
> -          aCallback();
> -        });
> +    is(tab.linkedBrowser.contentWindow.location, NAVIGATE_URL,
> +      "Target started navigating back to the previous location.");
> -      });
>  
> -      aDebuggee.location = SIMPLE_URL;
> +    yield onDidNav;
> +    is(tab.linkedBrowser.contentWindow.location, SIMPLE_URL,

CPOW?

::: devtools/client/netmonitor/test/browser_net_persistent_logs.js:21
(Diff revision 1)
> -    reqMenu = RequestsMenu;
>  
> -    Services.prefs.setBoolPref("devtools.webconsole.persistlog", false);
> +  Services.prefs.setBoolPref("devtools.webconsole.persistlog", false);
> -    content.location.reload(true);
> -  })
> -  .then(() => {
> +
> +  tab.linkedBrowser.reload(true);
> +  yield waitForNetworkEvents(monitor, 2);

Listen for events before reloading.

::: devtools/client/netmonitor/test/browser_net_persistent_logs.js:27
(Diff revision 1)
> -  .then(() => {
> -    is(reqMenu.itemCount, 2,
> -      "The request menu should have two items at this point.");
> +    "The request menu should have two items at this point.");
> -  })
> -  .then(() => {
> -    content.location.reload(true);
> +
> +  tab.linkedBrowser.reload(true);
> +  yield waitForNetworkEvents(monitor, 2);

Same

::: devtools/client/netmonitor/test/browser_net_persistent_logs.js:37
(Diff revision 1)
> -  .then(() => {
> -    // Now we toggle the persistence logs on
> +  // Now we toggle the persistence logs on
> -    Services.prefs.setBoolPref("devtools.webconsole.persistlog", true);
> +  Services.prefs.setBoolPref("devtools.webconsole.persistlog", true);
> -    content.location.reload(true);
> -    return waitForNetworkEvents(monitor, 2);
> -  })
> +
> +  tab.linkedBrowser.reload(true);
> +  yield waitForNetworkEvents(monitor, 2);

Same

::: devtools/client/netmonitor/test/browser_net_persistent_logs.js:44
(Diff revision 1)
> -    is(reqMenu.itemCount, 4,
> +  is(RequestsMenu.itemCount, 4,
> -      "The request menu should now have four items at this point.");
> +    "The request menu should now have four items at this point.");
> -  })
> +
> -  .then(() => {
> -    Services.prefs.setBoolPref("devtools.webconsole.persistlog", false);
> +  Services.prefs.setBoolPref("devtools.webconsole.persistlog", false);
> -    return teardown(monitor).then(finish);
> +  return teardown(monitor);

yield teardown(monitor); ?

::: devtools/client/netmonitor/test/browser_net_post-data-01.js:50
(Diff revision 1)
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.getElementById("details-pane-toggle"));
> +    document.getElementById("details-pane-toggle"));
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[2]);
> +    document.querySelectorAll("#details-pane tab")[2]);
>  
> -      let TAB_UPDATED = aMonitor.panelWin.EVENTS.TAB_UPDATED;
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

Listen for events before the mouse events.

::: devtools/client/netmonitor/test/browser_net_post-data-01.js:54
(Diff revision 1)
> -      let TAB_UPDATED = aMonitor.panelWin.EVENTS.TAB_UPDATED;
> -      waitFor(aMonitor.panelWin, TAB_UPDATED).then(() =>
> -        testParamsTab("urlencoded")
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);
> +  yield testParamsTab("urlencoded");
> +
> -      ).then(() => {
> -        RequestsMenu.selectedIndex = 1;
> +  RequestsMenu.selectedIndex = 1;
> -        return waitFor(aMonitor.panelWin, TAB_UPDATED);
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

Listen for tab updated before selectedIndex=1

::: devtools/client/netmonitor/test/browser_net_post-data-01.js:57
(Diff revision 1)
> -      ).then(() => {
> -        RequestsMenu.selectedIndex = 1;
> +  RequestsMenu.selectedIndex = 1;
> -        return waitFor(aMonitor.panelWin, TAB_UPDATED);
> -      }).then(() => testParamsTab("multipart"))
> -        .then(() => teardown(aMonitor))
> -        .then(finish);
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);
> +  yield testParamsTab("multipart");
> +
> +  return teardown(monitor);

yield teardown?

::: devtools/client/netmonitor/test/browser_net_post-data-02.js:30
(Diff revision 1)
> +  yield wait;
> +
> -      NetMonitorView.toggleDetailsPane({ visible: true }, 2);
> +  NetMonitorView.toggleDetailsPane({ visible: true }, 2);
> -      RequestsMenu.selectedIndex = 0;
> +  RequestsMenu.selectedIndex = 0;
>  
> -      let TAB_UPDATED = aMonitor.panelWin.EVENTS.TAB_UPDATED;
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

Listen for tab updated before selectedIndex=0

::: devtools/client/netmonitor/test/browser_net_post-data-02.js:70
(Diff revision 1)
> -          "baz", "The second query param name was incorrect.");
> +    "baz", "The second query param name was incorrect.");
> -        is(postScope.querySelectorAll(".variables-view-variable .value")[1].getAttribute("value"),
> +  is(postScope.querySelectorAll(".variables-view-variable .value")[1]
> +    .getAttribute("value"),
> -          "\"123\"", "The second query param value was incorrect.");
> +    "\"123\"", "The second query param value was incorrect.");
>  
> -        teardown(aMonitor).then(finish);
> +  return teardown(monitor);

nit: yield teardown?

::: devtools/client/netmonitor/test/browser_net_post-data-03.js:29
(Diff revision 1)
> +  yield wait;
>  
> -      NetMonitorView.toggleDetailsPane({ visible: true });
> +  NetMonitorView.toggleDetailsPane({ visible: true });
> -      RequestsMenu.selectedIndex = 0;
> +  RequestsMenu.selectedIndex = 0;
>  
> -      yield waitFor(aMonitor.panelWin, TAB_UPDATED);
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

Listen for tab updated before selectedIndex=0

::: devtools/client/netmonitor/test/browser_net_post-data-03.js:95
(Diff revision 1)
> -        "baz", "The second payload param name was incorrect.");
> +    "baz", "The second payload param name was incorrect.");
> -      is(formDataScope.querySelectorAll(".variables-view-variable .value")[1].getAttribute("value"),
> +  is(formDataScope.querySelectorAll(".variables-view-variable .value")[1]
> +    .getAttribute("value"),
> -        "\"123\"", "The second payload param value was incorrect.");
> +    "\"123\"", "The second payload param value was incorrect.");
>  
> -      teardown(aMonitor).then(finish);
> +  return teardown(monitor);

nit: yield teardown?

::: devtools/client/netmonitor/test/browser_net_prefs-and-l10n.js:22
(Diff revision 1)
> -      "Should have a preferences object available on the panel window.");
> +    "Should have a preferences object available on the panel window.");
>  
> +  testL10N();
> +  testPrefs();
> +
> +  return teardown(monitor);

nit: yield teardown?

::: devtools/client/netmonitor/test/browser_net_prefs-reload.js:53
(Diff revision 1)
> +  yield testSide();
> +  yield testWindow();
> +
> +  info("Moving toolbox back to the bottom...");
> +  yield monitor._toolbox.switchHost(Toolbox.HostType.BOTTOM);
> +  return teardown(monitor);

nit: yield teardown?

::: devtools/client/netmonitor/test/browser_net_prefs-reload.js:150
(Diff revision 1)
> -      // Validate and modify while toolbox is on the bottom.
> +    // Validate and modify while toolbox is on the bottom.
> -      validateFirstPrefValues();
> +    validateFirstPrefValues();
> -      modifyFrontend();
> +    modifyFrontend();
>  
> -      return restartNetMonitor(aMonitor)
> -        .then(([,, aNewMonitor]) => {
> +    let [,, newMonitor] = yield restartNetMonitor(monitor);
> +    monitor = newMonitor;

I think you can do:
[,, monitor] = yield restartNetMonitor(monitor);
or may be:
([,, monitor]) = yield restartNetMonitor(monitor);
Attachment #8785302 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #24)
> > +    is(tab.linkedBrowser.contentWindow.location, SIMPLE_URL,
> 
> Note that this is a possible CPOW. (linkedBrowser.contentWindow)

Is it really? I thought that only the contentWindow.wrappedJSObject is a CPOW. Is there some parent-side-only instance of the Window that has the "location" property and doesn't need to talk to the content process over a CPOW? I don't know exactly how the various Window objects (outer, inner, parent, content) work.

> > +  return teardown(monitor);
> 
> yield teardown(monitor); ?

I'm choosing "return" here to indicate that the test execution is ending here and what follows are only function definitions (going to be hoisted up).

> I think you can do:
> [,, monitor] = yield restartNetMonitor(monitor);
> or may be:
> ([,, monitor]) = yield restartNetMonitor(monitor);

Interesting, I thought destructuring works only with "let". Turns out it works with arrays, too ([i] = [1]), but doesn't work with objects: {m} = { m: 1 }.

Anyway, I'm converting the initNetMonitor return value to object in the last patch, so this code gets more elegant (no commas)
Blocks: 1077403
Comment on attachment 8785303 [details]
Bug 1297362 - Part 5: Eliminate CPOWs from Netmonitor mochitests R-T

https://reviewboard.mozilla.org/r/74558/#review72744

::: devtools/client/netmonitor/test/browser_net_raw_headers.js:28
(Diff revision 1)
> +  yield wait;
> +
> -      let origItem = RequestsMenu.getItemAtIndex(0);
> +  let origItem = RequestsMenu.getItemAtIndex(0);
> -      RequestsMenu.selectedItem = origItem;
> +  RequestsMenu.selectedItem = origItem;
>  
> -      waitFor(aMonitor.panelWin, TAB_UPDATED).then(() => {
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

Wait for tab updated before selectedItem = origItem

::: devtools/client/netmonitor/test/browser_net_reload-button.js:20
(Diff revision 1)
> -    reqMenu = RequestsMenu;
>  
> -    let button = document.querySelector("#requests-menu-reload-notice-button");
> +  let button = document.querySelector("#requests-menu-reload-notice-button");
> -    button.click();
> +  button.click();
> -  })
> -  .then(() => {
> +
> +  yield waitForNetworkEvents(monitor, 2);

Listen for events before clicking

::: devtools/client/netmonitor/test/browser_net_resend.js:32
(Diff revision 1)
> +  });
> +  yield wait;
> +
> -      let origItem = RequestsMenu.getItemAtIndex(0);
> +  let origItem = RequestsMenu.getItemAtIndex(0);
> -      RequestsMenu.selectedItem = origItem;
> +  RequestsMenu.selectedItem = origItem;
> +  yield panelWin.once(EVENTS.TAB_UPDATED);

Listen for tab updated before selectedItem = origItem

::: devtools/client/netmonitor/test/browser_net_resend.js:36
(Diff revision 1)
> +  yield panelWin.once(EVENTS.TAB_UPDATED);
>  
> -      waitFor(aMonitor.panelWin, TAB_UPDATED).then(() => {
> -        // add a new custom request cloned from selected request
> +  // add a new custom request cloned from selected request
> -        RequestsMenu.cloneSelectedRequest();
> +  RequestsMenu.cloneSelectedRequest();
> -        return waitFor(aMonitor.panelWin, CUSTOMREQUESTVIEW_POPULATED);
> +  yield panelWin.once(EVENTS.CUSTOMREQUESTVIEW_POPULATED);

Similar

::: devtools/client/netmonitor/test/browser_net_security-tab-visibility.js:42
(Diff revision 1)
>    RequestsMenu.lazyUpdate = false;
>  
>    for (let testcase of TEST_DATA) {
>      info("Testing Security tab visibility for " + testcase.desc);
>      let onNewItem = monitor.panelWin.once(EVENTS.NETWORK_EVENT);
> -    let onSecurityInfo = monitor.panelWin.once(EVENTS.RECEIVED_SECURITY_INFO);
> +    let onSecurityInfo = monitor.panelWin.once(EVENTS.UPDATING_SECURITY_INFO);

Is this change expected?
It doesn't look related to CPOW removal nor eslint cleanup

::: devtools/client/netmonitor/test/browser_net_simple-init.js:10
(Diff revision 1)
> +
>  /**
>   * Simple check if the network monitor starts up and shuts down properly.
>   */
>  
>  function test() {

no add_task for this test?

::: devtools/client/netmonitor/test/browser_net_simple-init.js:19
(Diff revision 1)
>    let gOk = ok;
>  
> -  initNetMonitor(SIMPLE_URL).then(([aTab, aDebuggee, aMonitor]) => {
> +  initNetMonitor(SIMPLE_URL).then(([tab, , monitor]) => {
>      info("Starting test... ");
>  
> -    is(aTab.linkedBrowser.contentWindow.wrappedJSObject.location, SIMPLE_URL,
> +    is(tab.linkedBrowser.contentWindow.location, SIMPLE_URL,

Possible CPOW via contentWindow attribute

::: devtools/client/netmonitor/test/browser_net_simple-request-data.js:10
(Diff revision 1)
> +
>  /**
>   * Tests if requests render correct information in the menu UI.
>   */
>  
>  function test() {

no task?

::: devtools/client/netmonitor/test/browser_net_simple-request-details.js:39
(Diff revision 1)
> -      is(RequestsMenu.selectedIndex, 0,
> +  is(RequestsMenu.selectedIndex, 0,
> -        "The first item should be selected in the requests menu.");
> +    "The first item should be selected in the requests menu.");
> -      is(NetMonitorView.detailsPaneHidden, false,
> +  is(NetMonitorView.detailsPaneHidden, false,
> -        "The details pane should not be hidden after toggle button was pressed.");
> +    "The details pane should not be hidden after toggle button was pressed.");
>  
> -      yield waitFor(aMonitor.panelWin, TAB_UPDATED);
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

This test looks weird. There is assertions just after the mousedown which seems to asynchronously fire the tab_updated.
First, we should listen for the event before starting the action triggering it.
Second, the assertion looks weird given the asynchrousity of this mousedown. So part of it's behavior is sync, but the tab_updated is fired async?
What about asserting after the tab_updated?

::: devtools/client/netmonitor/test/browser_net_simple-request-details.js:153
(Diff revision 1)
>  
> -    function testCookiesTab() {
> +  function* testCookiesTab() {
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +    EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[1]);
> +      document.querySelectorAll("#details-pane tab")[1]);
>  
> -      return Task.spawn(function* () {
> +    yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

We should listen before the mousedown

::: devtools/client/netmonitor/test/browser_net_simple-request-details.js:196
(Diff revision 1)
>  
> -    function testResponseTab() {
> +  function* testResponseTab() {
> -      EventUtils.sendMouseEvent({ type: "mousedown" },
> +    EventUtils.sendMouseEvent({ type: "mousedown" },
> -        document.querySelectorAll("#details-pane tab")[3]);
> +      document.querySelectorAll("#details-pane tab")[3]);
>  
> -      return Task.spawn(function* () {
> +    yield monitor.panelWin.once(EVENTS.TAB_UPDATED);

Same.

::: devtools/client/netmonitor/test/browser_net_simple-request.js:70
(Diff revision 1)
> -      });
>  
> -      aDebuggee.location.reload();
> -    });
> -
> -    aDebuggee.location.reload();
> +  function* reloadAndWait() {
> +    let wait = waitForNetworkEvents(monitor, 1);
> +    tab.linkedBrowser.reload();
> +    return wait;

I'm wondering if that's something you can share with other netmon tests?

::: devtools/client/netmonitor/test/browser_net_sort-02.js:44
(Diff revision 1)
> -    }];
> +  }];
>  
> -    RequestsMenu.lazyUpdate = false;
> +  RequestsMenu.lazyUpdate = false;
>  
> -    waitForNetworkEvents(aMonitor, 5).then(() => {
> +  let wait = waitForNetworkEvents(monitor, 5);
> +  yield performRequestsInContent(requests);

In another test you had to call loadCommonFrameScript,
shouldn't we do the same here?

::: devtools/client/netmonitor/test/browser_net_sort-03.js:44
(Diff revision 1)
> -    }];
> +  }];
>  
> -    RequestsMenu.lazyUpdate = false;
> +  RequestsMenu.lazyUpdate = false;
>  
> -    waitForNetworkEvents(aMonitor, 5).then(() => {
> +  let wait = waitForNetworkEvents(monitor, 5);
> +  yield performRequestsInContent(requests);

loadCommandFrameScript?

::: devtools/client/netmonitor/test/browser_net_status-codes.js:208
(Diff revision 1)
>     * A helper that clicks on a specified request and returns a promise resolved
>     * when NetworkDetails has been populated with the data of the given request.
>     */
>    function chooseRequest(index) {
>      EventUtils.sendMouseEvent({ type: "mousedown" }, requestItems[index].target);
> -    return waitFor(monitor.panelWin, monitor.panelWin.EVENTS.TAB_UPDATED);
> +    return monitor.panelWin.once(EVENTS.TAB_UPDATED);

Ideally we would wait for the event before the mousedown
Attachment #8785303 - Flags: review?(poirot.alex) → review+
(In reply to Jarda Snajdr [:jsnajdr] from comment #25)
> (In reply to Alexandre Poirot [:ochameau] from comment #24)
> > > +    is(tab.linkedBrowser.contentWindow.location, SIMPLE_URL,
> > 
> > Note that this is a possible CPOW. (linkedBrowser.contentWindow)
> 
> Is it really? I thought that only the contentWindow.wrappedJSObject is a
> CPOW.

Both contentWindow and the wrappedJSObject of it should be CPOW. The first is just the xray wrapper which only exposes DOM natives plus expandos from chrome principal.

> Is there some parent-side-only instance of the Window that has the
> "location" property and doesn't need to talk to the content process over a
> CPOW? I don't know exactly how the various Window objects (outer, inner,
> parent, content) work.

I don't think so. Any "window" related to the document lives in the content process.
There is just some stuff sometimes mirrored in the parent mostly via frame scripts.
<xul:browser> is implemented by this XBL binding:
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml
But for remote tab, there is XBL binding on top of the previous:
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml

You should have linkedBrowser.documentURI for this usecase:
 https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#156

But there is something I don't get. It looks like linkedBrowser.contentWindow should be null for out of process tabs:
  https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#156
So may be you don't get a CPOW here as the tab isn't running OOP?


> 
> > > +  return teardown(monitor);
> > 
> > yield teardown(monitor); ?
> 
> I'm choosing "return" here to indicate that the test execution is ending
> here and what follows are only function definitions (going to be hoisted up).

Ok ok, feel free to ignore all my comments about this. It was mostly for consistency.
But it looks like you are using this pattern often enough to set a new consistency ;)
Comment on attachment 8785304 [details]
Bug 1297362 - Part 6: Eliminate CPOWs from Netmonitor mochitests HAR

https://reviewboard.mozilla.org/r/74560/#review72764
Attachment #8785304 - Flags: review?(poirot.alex) → review+
Comment on attachment 8785305 [details]
Bug 1297362 - Part 7: Stop creating CPOWs in netmonitor/test/head.js

https://reviewboard.mozilla.org/r/74562/#review72768

Thanks for the distinct changeset for this, it is really handy to review!
Feel free to merge patches before landing to whatever makes sense to you.
Attachment #8785305 - Flags: review?(poirot.alex) → review+
Comment on attachment 8785303 [details]
Bug 1297362 - Part 5: Eliminate CPOWs from Netmonitor mochitests R-T

https://reviewboard.mozilla.org/r/74558/#review72744

> Is this change expected?
> It doesn't look related to CPOW removal nor eslint cleanup

Oops, good catch! This was a change planned for bug 1176050, doesn't belong here. It wouldn't cause any damage though, because the test is only checking securityState, which is available already on UPDATING_SECURITY_INFO. Only securityInfo (the details) needs to wait for RECEIVED_SECURITY_INFO.

> no add_task for this test?

This test's structure doesn't fit so well into a linear Task.js structure, so I kept promises there. There's the executeSoon which I'm not sure if it's necessary. And an important part of the test is executed after the Netmonitor shutdown.

> no task?

Again, the test structure is not linear. All the event listener wait and execute in parallel, and waitForNetworkEvents internally wait for all these events, too, so it's guaranteed that all the listeners are fired and executed before the test is finished.

> This test looks weird. There is assertions just after the mousedown which seems to asynchronously fire the tab_updated.
> First, we should listen for the event before starting the action triggering it.
> Second, the assertion looks weird given the asynchrousity of this mousedown. So part of it's behavior is sync, but the tab_updated is fired async?
> What about asserting after the tab_updated?

This test "abuses" the insider knowledge about what happens synchronously and asynchronously after the details pane is toggled.

The details pane is displayed and the first item is selected synchronously, but the tab contents are displayed async, mainly because the request/response body longStrings are fetched lazily from the server.

I'll change the test to wait before checking anything, making it more robust against future implementation changes.

> I'm wondering if that's something you can share with other netmon tests?

I totally agree, but I'd like to do that (any many other similar improvements) in a separate follow-up bug. I'm afraid this patch will get out of control soon :)

> In another test you had to call loadCommonFrameScript,
> shouldn't we do the same here?

It is loaded, only several lines earlier. Both in sort-02 and sort-03.
(In reply to Alexandre Poirot [:ochameau] from comment #27)
> You should have linkedBrowser.documentURI for this usecase:
>  https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> remote-browser.xml#156

I switched to tab.linkedBrowser.currentURI, because that's what add-on SDK is using:

http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/tabs/utils.js#278

> But there is something I don't get. It looks like
> linkedBrowser.contentWindow should be null for out of process tabs:

No idea about this either, but contentWindow seems to be accessible even in e10s.
Resolved the review issues:
- start waiting for completion event before clicking on something in UI (some places probably still remain, will be eventually fixed)
- don't use CPOW to get tab location - use browser.currentURI instead

One more try run and I'll land this.
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b596aea79722
Part 1: Eliminate CPOWs from Netmonitor mochitests A r=Honza,ochameau
https://hg.mozilla.org/integration/autoland/rev/1e3a17dd7c5f
Part 2: Eliminate CPOWs from Netmonitor mochitests C r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6c64ca4b0b16
Part 3: Eliminate CPOWs from Netmonitor mochitests D-J r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d0bf47d5fc30
Part 4: Eliminate CPOWs from Netmonitor mochitests L-P r=ochameau
https://hg.mozilla.org/integration/autoland/rev/dc188ac077e9
Part 5: Eliminate CPOWs from Netmonitor mochitests R-T r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a3467eb2df37
Part 6: Eliminate CPOWs from Netmonitor mochitests HAR r=ochameau
https://hg.mozilla.org/integration/autoland/rev/03104edc024d
Part 7: Stop creating CPOWs in netmonitor/test/head.js r=ochameau
Blocks: 1297891
Blocks: 1298585
Blocks: 1297274
Blocks: 1296228
Blocks: 1296226
Blocks: 1295846
Blocks: 1295438
Blocks: 1295430
Blocks: 1293216
Blocks: 1292987
Blocks: 1289365
Blocks: 1289043
Blocks: 1285714
Blocks: 1285626
Blocks: 1284685
Blocks: 1280007
Blocks: 1278487
Blocks: 1277839
Blocks: 1277655
Blocks: 1276733
Blocks: 1276410
Blocks: 1274778
Blocks: 1267352
Duplicate of this bug: 1124330
Blocks: 1242204
Blocks: 1150206
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.