Bug 1609701 Comment 2 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Original comment by
on 
This regression was introduced with bug 1578942 where we started suspending timeouts for inactive sessions.

However, our mechanics around timeout suspension seem sensible.

For Gecko devs:
1. `GeckoSession.setActive(false)`
  a. `browser.docShellIsActive = false`
  b. `docShell.contentViewer.pausePainting()`
  c. `content.windowUtils.suspendTimeouts()`
2. `GeckoSession.reload()`
  a. `browser.reload();`
3. `GeckoSession.setActive(true)`
  a. `browser.docShellIsActive = true`
  b. `docShell.contentViewer.resumePainting()`
  c. `content.windowUtils.resumeTimeouts()`

With `3.c` we end up calling  `nsGlobalWindowInner::Resume` on an inner window with `mSuspendDepth == 0` which fails horribly.
It looks like resuming timeouts (`nsDOMWindowUtils::ResumeTimeouts` -> `nsGlobalWindowInner::Resume`) expects to only be called on suspended windows and I don't see how that could be enforced given that it's an unconstrained JS API.

Unless there are good reasons that the assumption needs to hold, I'm proposing to just ignore resume on non-suspended windows.
Revision 1 by
on 
This regression was introduced with bug 1578942 where we started suspending timeouts for inactive sessions.

However, our mechanics around timeout suspension seem sensible.

For Gecko devs:
1. `GeckoSession.setActive(false)`
  a. `browser.docShellIsActive = false`
  b. `docShell.contentViewer.pausePainting()`
  c. `content.windowUtils.suspendTimeouts()`
2. `GeckoSession.reload()`
  a. `browser.reload();`
3. `GeckoSession.setActive(true)`
  a. `browser.docShellIsActive = true`
  b. `docShell.contentViewer.resumePainting()`
  c. `content.windowUtils.resumeTimeouts()`

With `3.c` we end up calling  `nsGlobalWindowInner::Resume` on an inner window with `mSuspendDepth == 0` which fails horribly.
It looks like resuming timeouts (`nsDOMWindowUtils::ResumeTimeouts` -> `nsGlobalWindowInner::Resume`) expects to only be called on suspended windows and I don't see how that could be enforced given that it's an unconstrained JS API.

Unless there are good reasons that the assumption needs to hold, I'm proposing to just ignore resuming on non-suspended windows.
Revision 2 by
on 
This regression was introduced with bug 1578942 where we started suspending timeouts for inactive sessions.
However, our mechanics around timeout suspension seem sensible.

For Gecko devs:
1. `GeckoSession.setActive(false)`
  a. `browser.docShellIsActive = false`
  b. `docShell.contentViewer.pausePainting()`
  c. `content.windowUtils.suspendTimeouts()`
2. `GeckoSession.reload()`
  a. `browser.reload();`
3. `GeckoSession.setActive(true)`
  a. `browser.docShellIsActive = true`
  b. `docShell.contentViewer.resumePainting()`
  c. `content.windowUtils.resumeTimeouts()`

With `3.c` we end up calling  `nsGlobalWindowInner::Resume` on an inner window with `mSuspendDepth == 0` which fails horribly.
It looks like resuming timeouts (`nsDOMWindowUtils::ResumeTimeouts` -> `nsGlobalWindowInner::Resume`) expects to only be called on suspended windows and I don't see how that could be enforced given that it's an unconstrained JS API.

Unless there are good reasons that the assumption needs to hold, I'm proposing to just ignore resuming on non-suspended windows.

Back to Bug 1609701 Comment 2