1505701 - Tab is closed before finish handling event in testcases

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Tooru Fujisawa [:arai]]

Attachment: trace from EventUtils.sendMouseEvent to executeSoon

steps to reproduce:
  1. apply bug 1498775 patch and build (which adds error reporting for unhandled promise rejection for more cases)
     this is just to cause the test failure, the issue itself happens even without it
  2. run the following tests:
    devtools/client/canvasdebugger/test/browser_canvas-frontend-call-stack-01.js
    devtools/client/inspector/markup/test/browser_markup_links_06.js

actual result:
  FAIL A promise chain failed to handle a rejection: [Exception... "Component not initialized"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: resource://devtools/client/debugger/new/src/utils/DevToolsUtils.js :: executeSoon :: line 24"  data: no] - stack: executeSoon@resource://devtools/client/debugger/new/src/utils/DevToolsUtils.js:24:3

expected result:
  tests pass

this happens because the tab is closed while handling setTimeout here:
https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/devtools/client/debugger/new/src/utils/DevToolsUtils.js#15
> export function executeSoon(fn: Function) {
>   setTimeout(fn, 0);
> }

and the failure is not handled here (the promise returned by `promiseInst.then` is just ignored.

https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/devtools/client/debugger/new/src/actions/utils/middleware/promise.js#86
>     return new Promise((resolve, reject) => {
>       promiseInst.then(
>         value => {
>           executeSoon(() => {
>             dispatch({ ...action, status: "done", value: value });
>             resolve(value);
>           });
>         },
>         error => {
>           executeSoon(() => {
>             dispatch({
>               ...action,
>               status: "error",
>               error: error.message || error
>             });
>             reject(error);
>           });
>         }
>       );

for browser_canvas-frontend-call-stack-01.js case, that code is called from here (see te attached trace)

https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/devtools/client/canvasdebugger/test/browser_canvas-frontend-call-stack-01.js#65-72
> async function ifTestingSupported() {
> ...
>   const jumpedToSource = once(window, EVENTS.SOURCE_SHOWN_IN_JS_DEBUGGER);
>   EventUtils.sendMouseEvent({ type: "mousedown" }, $(".call-item-stack-fn-location", callItem.target));
>   await jumpedToSource;
> 
>   const toolbox = await gDevTools.getToolbox(target);
>   const dbg = createDebuggerContext(toolbox);
>   await validateDebuggerLocation(dbg, SIMPLE_CANVAS_DEEP_STACK_URL, 26);
> 
>   await teardown(panel);

EventUtils.sendMouseEvent triggers the executeSoon, but the `jumpedToSource` is resolved before that point, and `teardown` there closes the tab, while the setTimeout is running.

I wonder where to apply fix... :/

Comment 1

[Tooru Fujisawa [:arai]]

simple workaround would be, just wait for finish of the handling in the testcase before closing tab.
another not-simple fix is to handle the rejection in middleware/promise.js, but not sure if this is the right place (I'm not sure what the whole library is for)

Comment 2

[Tooru Fujisawa [:arai]]

:jlast, can I have your opinion where to fix this case?

Comment 3

[Jason Laster [:jlast]]

hmm, i'm curious what logan and nicolas think. 

I think in the interest of landng the  error reporting for unhandled promise rejection fix, we should do the smallest test fix. We will have plenty of other failures with this added check and can implement more architectural fixes then :)

Comment 4

[Logan Smyth [:loganfsmyth]]

That middleware seems like a classic example of a broken promise chain. If we refactor it to

```
return promiseInst.then(
  value => new Promise(resolve => {
    executeSoon(() => {
      dispatch({ ...action, status: "done", value: value });
      resolve(value);
    });
  }),
  error => new Promise((resolve, reject) => {
    executeSoon(() => {
      dispatch({
        ...action,
        status: "error",
        error: error.message || error
      });
      reject(error);
    });
  })
);
```

the error will propagate through the promise chain properly.

That said, if we're refactoring anyway, it's still a little dangerous to call `dispatch` like that because if `dispatch` throws, we'd end up with an uncaught exception instead of a rejected promise. In my experience, the body of a `new Promise` executor should be as small as possible, so I'd probably refactor this to be
```
return promiseInst.then(
  value => new Promise(res => executeSoon(res)).then(() => {
    dispatch({ ...action, status: "done", value: value });
    return value;
  }),
  error => new Promise(res => executeSoon(res)).then(()
    dispatch({
      ...action,
      status: "error",
      error: error.message || error
    });
    return Promise.reject(error);
  })
);

```
or even better
```
return Promise.resolve(promiseInst)
  .finally(() => new Promise(res => executeSoon(res))
  .then(
    value => {
      dispatch({ ...action, status: "done", value: value });
      return value;
    },
    error => {
      dispatch({
        ...action,
        status: "error",
        error: error.message || error
      });
      return Promise.reject(error);
    }
  );
```

Comment 5

[Tooru Fujisawa [:arai]]

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

I'll open PR

Comment 6

[Tooru Fujisawa [:arai]]

opened https://github.com/devtools-html/debugger.html/pull/7276

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

arai, the PR was merged (and other similar ones too if I remember correctly).
Is there anything more to do here?

Comment 8

[Jason Laster [:jlast]]

I believe this is fixed.

Comment 9

[Tooru Fujisawa [:arai]]

Thanks!
yes, it's fixed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11ef53a2d1d9c61227dad4c3d151ab06739b3834&group_state=expanded