Closed Bug 1439471 Opened 6 years ago Closed 6 years ago

devtools/client/netmonitor/test/browser_net_status-bar.js fails after bug 1193394

Categories

(DevTools :: Netmonitor, defect)

defect
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 1 obsolete file)

devtools/client/netmonitor/test/browser_net_status-bar.js

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9653afa89f067d4a8146ffab41835e9153808fd&group_state=expanded&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=163037154
> 16:14:04    ERROR -  540 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_status-bar.js | There must be load label -
> 16:14:04     INFO -  Stack trace:
> 16:14:04     INFO -  chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_status-bar.js:null:35
> 16:14:04     INFO -  541 INFO TEST-PASS | devtools/client/netmonitor/test/browser_net_status-bar.js | There must be request count label text -
> 16:14:04     INFO -  542 INFO TEST-PASS | devtools/client/netmonitor/test/browser_net_status-bar.js | There must be size label text -
> 16:14:04     INFO -  543 INFO TEST-PASS | devtools/client/netmonitor/test/browser_net_status-bar.js | There must be DOMContentLoaded label text -
> 16:14:04     INFO -  Not taking screenshot here: see the one that was previously logged
> 16:14:04    ERROR -  544 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_status-bar.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_status-bar.js:48 - TypeError: onLoad is null
> 16:14:04     INFO -  Stack trace:
> 16:14:04     INFO -      @chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_status-bar.js:48:25
> 16:14:04     INFO -      waitUntil@chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js:471:7
> 16:14:04     INFO -      @chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_status-bar.js:48:9
> 16:14:04     INFO -      Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1067:21
> 16:14:04     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
> 16:14:04     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:958:9
> 16:14:04     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
reproduced locally.
taking.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
waitForTimelineMarkers waits for EVENTS.TIMELINE_EVENT, that is emitted in the 1st line, but StatusBar is updated in the 2nd line below:

https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/devtools/client/netmonitor/src/connector/firefox-connector.js#215-216
>   onDocEvent(type, event) {
>     window.emit(EVENTS.TIMELINE_EVENT, event);
>     this.actions.addTimingMarker(event);
>   }

Once bug 1193394 is fixed, promise resolution handler for the promise resolved inside the 1st line can be executed before StatusBar is updated inside the 2nd line, if there's a MicroTask checkpoint somewhere between the promise resolution and StatusBar update.
I haven't yet found the exact place of the MicroTask checkpoint tho, I suspect it's related to XBL (see bug 1416153 comment #4,
basically there shouldn't be a MicroTask checkpoint inside sync JS code, but XBL accessors contain it)

Anyway, currently the promise returned by waitForTimelineMarkers is resolved before the StatusBar is updated, and the remaining part after `await Promise.all([requestsDone, markersDone]);` can be executed before `onLoad` element is created once bug 1193394 is fixed.

I added `await new Promise(resolve => executeSoon(resolve));` after that to wait for an event tick, to make sure StatusBar is updated before getting `onLoad` element.
Attachment #8952296 - Flags: review?(odvarko)
Comment on attachment 8952296 [details] [diff] [review]
Wait for the next event tick after doc-complete marker to make sure the StatusBar is updated.

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

Thanks for working on this!

I would prefer change both `onDocLoadingMarker` and `onDocEvent` as follows, which sounds more correct to me.

Honza

diff --git a/devtools/client/netmonitor/src/connector/firefox-connector.js b/devtools/client/netmonitor/src/connector/firefox-connector.js
--- a/devtools/client/netmonitor/src/connector/firefox-connector.js
+++ b/devtools/client/netmonitor/src/connector/firefox-connector.js
@@ -195,30 +195,30 @@ class FirefoxConnector {
   onDocLoadingMarker(marker) {
     // Translate marker into event similar to newer "docEvent" event sent by the console
     // actor
     let event = {
       name: marker.name == "document::DOMContentLoaded" ?
             "dom-interactive" : "dom-complete",
       time: marker.unixTime / 1000
     };
+    this.actions.addTimingMarker(event);
     window.emit(EVENTS.TIMELINE_EVENT, event);
-    this.actions.addTimingMarker(event);
   }
 
   /**
    * The "DOMContentLoaded" and "Load" events sent by the console actor.
    *
    * Only used by FF60+.
    *
    * @param {object} marker
    */
   onDocEvent(type, event) {
+    this.actions.addTimingMarker(event);
     window.emit(EVENTS.TIMELINE_EVENT, event);
-    this.actions.addTimingMarker(event);
   }
 
   /**
    * Send a HTTP request data payload
    *
    * @param {object} data data payload would like to sent to backend
    * @param {function} callback callback will be invoked after the request finished
    */
Attachment #8952296 - Flags: review?(odvarko)
Thanks!
it solves the issue.
Attachment #8952296 - Attachment is obsolete: true
Attachment #8952941 - Flags: review?(odvarko)
Comment on attachment 8952941 [details] [diff] [review]
Emit EVENTS.TIMELINE_EVENT after adding timing marker.

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

Thanks for the patch!
R+

Honza
Attachment #8952941 - Flags: review?(odvarko) → review+
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b263e3c54ee
Emit EVENTS.TIMELINE_EVENT after adding timing marker. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b263e3c54ee64f59dc0e33633590fca3fc614e3
Bug 1439471 - Emit EVENTS.TIMELINE_EVENT after adding timing marker. r=Honza
https://hg.mozilla.org/mozilla-central/rev/4b263e3c54ee
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: