Closed Bug 1129116 Opened 5 years ago Closed 5 years ago

Intermittent browser_net_filter-02.js | A promise chain failed to handle a rejection: - undefined | Test timed out

Categories

(DevTools :: Netmonitor, defect)

x86
Linux
defect
Not set

Tracking

(firefox36 unaffected, firefox37 unaffected, firefox38 fixed, firefox-esr31 unaffected)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox36 --- unaffected
firefox37 --- unaffected
firefox38 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: sjakthol)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

02:12:04 INFO - 4597 INFO Displayed status: 200
02:12:04 INFO - 4598 INFO Displayed code: 200
02:12:04 INFO - 4599 INFO Tooltip status: 200 OK
02:12:04 INFO - 4600 INFO TEST-PASS | browser/devtools/netmonitor/test/browser_net_filter-02.js | The displayed status is incorrect.
02:12:04 INFO - 4601 INFO TEST-PASS | browser/devtools/netmonitor/test/browser_net_filter-02.js | The displayed status code is incorrect.
02:12:04 INFO - 4602 INFO TEST-PASS | browser/devtools/netmonitor/test/browser_net_filter-02.js | The tooltip status is incorrect.
02:12:04 INFO - 4603 INFO Displayed type: x-shockwave-flash
02:12:04 INFO - 4604 INFO Tooltip type: application/x-shockwave-flash
02:12:04 INFO - 4605 INFO TEST-PASS | browser/devtools/netmonitor/test/browser_net_filter-02.js | The displayed type is incorrect.
02:12:04 INFO - 4606 INFO TEST-PASS | browser/devtools/netmonitor/test/browser_net_filter-02.js | The tooltip type is incorrect.
02:12:04 INFO - 4607 INFO Performing more requests.
02:12:04 INFO - 4608 INFO Console message: [JavaScript Warning: "unsafe CPOW usage" {file: "chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_filter-02.js" line: 50}]
02:12:04 INFO - 4609 INFO Console message: [JavaScript Error: "Error: operation not possible on dead CPOW"]
02:12:04 INFO - 4610 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_filter-02.js | A promise chain failed to handle a rejection: - undefined
02:12:04 INFO - Stack trace:
02:12:04 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 162
02:12:04 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 675
02:12:04 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 903
02:12:04 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 746
02:12:04 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.scheduleWalkerLoop/< :: line 688
02:12:04 INFO - native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
02:12:36 INFO - 4611 INFO Longer timeout required, waiting longer... Remaining timeouts: 1
02:13:21 INFO - Xlib: extension "RANDR" missing on display ":0".
02:13:25 INFO - TEST-INFO | screentopng: exit 0
02:13:25 INFO - 4612 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_filter-02.js | Test timed out - expected PASS
02:13:25 INFO - 4613 INFO finish() was called, cleaning up...
02:13:25 INFO - 4614 INFO MEMORY STAT vsize after test: 599838720
02:13:25 INFO - 4615 INFO MEMORY STAT residentFast after test: 155619328
02:13:25 INFO - 4616 INFO MEMORY STAT heapAllocated after test: 60790104
02:13:25 INFO - 4617 INFO TEST-OK | browser/devtools/netmonitor/test/browser_net_filter-02.js | took 90158ms
02:13:25 INFO - 4618 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_filter-02.js | Found a tab after previous test timed out: http://example.com/browser/browser/devtools/netmonitor/test/html_filter-test-page.html - expected PASS
As the logs contain a line "Error: operation not possible on dead CPOW" it seems that directly accessing the debuggee to trigger requests doesn't always work.

Here's a patch that adds support for triggering arbitrary XHRs in the content process to the devtools-wide frame-script-utils.js. This way tests can perform requests without using the debuggee CPOW.
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8561077 - Flags: review?(vporof)
This patch makes adds a wrapper for triggering requests with the frame script to head.js and makes browser_net_filter-0?.js tests to use the script.

This should take care of the errors caused by dead CPOW.
Attachment #8561079 - Flags: review?(vporof)
Comment on attachment 8561077 [details] [diff] [review]
devtools-framescript-add-xhr-request-support.patch

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

::: browser/devtools/shared/frame-script-utils.js
@@ +49,5 @@
> +  if (data.nocache) {
> +    url += "?devtools-cachebust=" + Math.random();
> +  }
> +
> +  let promise = new content.Promise(resolve => {

Any reason for building a content promise, instead of just a Promise?

@@ +53,5 @@
> +  let promise = new content.Promise(resolve => {
> +    xhr.addEventListener("loadend", function loadend(event) {
> +      xhr.removeEventListener("loadend", loadend);
> +      if (data.includeResponse) {
> +        resolve({ status: xhr.status, response: xhr.response });

Do we really need `includeResponse`? Why not always resolve with it?

@@ +58,5 @@
> +      } else {
> +        resolve({ status: xhr.status });
> +      }
> +    });
> +  });

Nit: Since this method does not reject on "error" or "abort", the returned promise always resolves. Maybe it would be nice if this method had a `timeout` param, that rejects the promise after a while. Not terribly important though.

@@ +91,5 @@
> + * }
> + */
> +addMessageListener("devtools:test:xhr", function ({ data }) {
> +  let requests = Array.isArray(data) ? data : [data];
> +  let sequence = content.Promise.resolve();

Ditto question for content.Promise.

@@ +102,5 @@
> +
> +      return responsePromise.then(response => {
> +        responsearray.push(response);
> +      });
> +    });

This loop would be so, so, so much nicer if you used Task. How about refactoring it a bit to yield for each request and then push the result in the `responsearray`.
Attachment #8561077 - Flags: review?(vporof) → review+
Comment on attachment 8561079 [details] [diff] [review]
netmonitor-filter-tests-avoid-cpow-usage.patch

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

::: browser/devtools/netmonitor/test/browser_net_filter-01.js
@@ +16,5 @@
> +  { url: "sjs_content-type-test-server.sjs?fmt=audio" },
> +  { url: "sjs_content-type-test-server.sjs?fmt=video" },
> +]);
> +
> +const REQUESTS_WITH_FLASH = REQUESTS_WITH_MEDIA.concat([

REQUESTS_WITH_MEDIA_AND_FLASH would be better

::: browser/devtools/netmonitor/test/browser_net_filter-02.js
@@ +16,5 @@
> +  { url: "sjs_content-type-test-server.sjs?fmt=audio" },
> +  { url: "sjs_content-type-test-server.sjs?fmt=video" },
> +]);
> +
> +const REQUESTS_WITH_FLASH = REQUESTS_WITH_MEDIA.concat([

Ditto.

::: browser/devtools/netmonitor/test/browser_net_filter-03.js
@@ +17,5 @@
> +  { url: "sjs_content-type-test-server.sjs?fmt=audio" },
> +  { url: "sjs_content-type-test-server.sjs?fmt=video" },
> +]);
> +
> +const REQUESTS_WITH_FLASH = REQUESTS_WITH_MEDIA.concat([

Ditto.

::: browser/devtools/netmonitor/test/browser_net_filter-04.js
@@ +17,5 @@
> +  { url: "sjs_content-type-test-server.sjs?fmt=audio" },
> +  { url: "sjs_content-type-test-server.sjs?fmt=video" },
> +]);
> +
> +const REQUESTS_WITH_FLASH = REQUESTS_WITH_MEDIA.concat([

Ditto.

::: browser/devtools/netmonitor/test/head.js
@@ +88,5 @@
>    targetWindow.focus();
>    let tab = targetBrowser.selectedTab = targetBrowser.addTab(aUrl);
>    let browser = tab.linkedBrowser;
>  
> +  browser.messageManager.loadFrameScript(FRAME_SCRIPT_UTILS_URL, false);

Maybe safer to load frame scripts on demand, instead of always doing it when a panel is created.
Attachment #8561079 - Flags: review?(vporof) → review+
Thanks for the review. Here's a version that addresses your comments. Carrying r+ over.
(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Comment on attachment 8561077 [details] [diff] [review]
> devtools-framescript-add-xhr-request-support.patch
> 
> Review of attachment 8561077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/frame-script-utils.js
> @@ +49,5 @@
> > +  if (data.nocache) {
> > +    url += "?devtools-cachebust=" + Math.random();
> > +  }
> > +
> > +  let promise = new content.Promise(resolve => {
> 
> Any reason for building a content promise, instead of just a Promise?
Promise does not seem to be available in the scope of the frame script. The script could import Promise.jsm (of course). Changed.

> @@ +53,5 @@
> > +  let promise = new content.Promise(resolve => {
> > +    xhr.addEventListener("loadend", function loadend(event) {
> > +      xhr.removeEventListener("loadend", loadend);
> > +      if (data.includeResponse) {
> > +        resolve({ status: xhr.status, response: xhr.response });
> 
> Do we really need `includeResponse`? Why not always resolve with it?
I wasn't sure about the performance impact it might have on e10s if the entire response body is transferred between processes. Though it doesn't really matter that much as this is only used in tests. Changed this to always include responses.

> @@ +58,5 @@
> > +      } else {
> > +        resolve({ status: xhr.status });
> > +      }
> > +    });
> > +  });
> 
> Nit: Since this method does not reject on "error" or "abort", the returned
> promise always resolves. Maybe it would be nice if this method had a
> `timeout` param, that rejects the promise after a while. Not terribly
> important though.
Didn't add it here but sounds like a good idea. Maybe it should be done once something requires it...

> @@ +102,5 @@
> > +
> > +      return responsePromise.then(response => {
> > +        responsearray.push(response);
> > +      });
> > +    });
> 
> This loop would be so, so, so much nicer if you used Task. How about
> refactoring it a bit to yield for each request and then push the result in
> the `responsearray`.
I always forget all the nice things that are available. Changed.
Attachment #8561077 - Attachment is obsolete: true
Attachment #8562043 - Flags: review+
(In reply to Victor Porof [:vporof][:vp] from comment #7)
> Comment on attachment 8561079 [details] [diff] [review]
> netmonitor-filter-tests-avoid-cpow-usage.patch
> 
> Review of attachment 8561079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/netmonitor/test/browser_net_filter-01.js
> @@ +16,5 @@
> > +  { url: "sjs_content-type-test-server.sjs?fmt=audio" },
> > +  { url: "sjs_content-type-test-server.sjs?fmt=video" },
> > +]);
> > +
> > +const REQUESTS_WITH_FLASH = REQUESTS_WITH_MEDIA.concat([
> 
> REQUESTS_WITH_MEDIA_AND_FLASH would be better
Changed everywhere.

> ::: browser/devtools/netmonitor/test/head.js
> @@ +88,5 @@
> >    targetWindow.focus();
> >    let tab = targetBrowser.selectedTab = targetBrowser.addTab(aUrl);
> >    let browser = tab.linkedBrowser;
> >  
> > +  browser.messageManager.loadFrameScript(FRAME_SCRIPT_UTILS_URL, false);
> 
> Maybe safer to load frame scripts on demand, instead of always doing it when
> a panel is created.
Changed.

Try run with both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93fe5a2a8bb4
Attachment #8561079 - Attachment is obsolete: true
Attachment #8562045 - Flags: review+
The oranges seem to be unrelated to my changes and seem to be caused by [1], fixed by [2].

Should be good to go.

[1] https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=dbe74171ca49
[2] https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=03733cfb9e9c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5736d0c605dc
https://hg.mozilla.org/mozilla-central/rev/0e4f7ef5a7c8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.