1303167 - nsGlobalWindow::SuspendTimeouts() is rather dubious

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

nsGlobalWindow has this method:

  virtual void SuspendTimeouts(uint32_t aIncrease = 1,
                               bool aFreezeChildren = true,
                               bool aFreezeWorkers = true) override;

It supports being called more then once.  It increments an mTimeoutsSuspendDepth counter each time its called.  It only actually does work when this counter transitions from 0 to greater than zero.

When a caller wants to stop suspending it uses this method:

  virtual nsresult ResumeTimeouts(bool aThawChildren = true,
                                  bool aThawWorkers = true) = 0;

Which decrements the count and resumes things when it drops to 0.

This is all well and good.  The dubious part is with mixing these de-duplication with modifying arguments.

SuspendTimeouts() takes two arguments indicating if child windows and workers should be frozen.  If they are false, then they are simply "suspended".  If they are true, then they are "frozen".

The code relies on the same arguments being passed to ResumeTimeouts() to resume and thaw as appropriate.

This is problematic with multiple callers using different arguments.  Consider:

  Client 1 calls SuspendTimeouts(1, true, true);
  Client 2 calls SuspendTimeouts(1, false, false);

This freezes both child windows and workers because the "true, true" version was called first.  The second call has no effect beyond incrementing the counter.

Next:

  Client 1 calls ResumeTimeouts(true, true);
  Client 2 calls ResumeTimeouts(false, false);

Here the "true, true" version has no effect because the counter is still non-zero.  When the "false, false" version is called we unsuspend things, but we don't thaw the frozen windows/workers!

Its unclear to me what the implication of unsuspending, but not thawing a frozen work is.  It seems like we should fix this API and code, though.

I need this to work properly in bug 1300659.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Some notes:

1) nsGlobalWindow::Freeze() fires an observer event and marks the window frozen.
2) Frozen windows are considered non-visible and can't be focused.
3) Most callers of SuspendTimeouts() do not freeze child windows.  Only the devtools and HandlePrerenderingViolation() freeze children.
4) Suspending a worker sets a flag that causes certain runnables to queued instead of dispatched to the worker thread.
5) Freezing a worker looks like it will actually interrupt JS a stop the worker thread until its thawed.
6) We tend to freeze workers whenever window timeouts are suspended.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #1)
> 6) We tend to freeze workers whenever window timeouts are suspended.

Correction.  Consumers of nsDOMWindowUtils (mainly devtools) does not freeze workers.  All other callers do seem to freeze the workers.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

The worker freezing was added for devtools in bug 1178721.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

The child freezing argument was added to SuspendTimeouts in bug 479490.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

ModalState stuff also does a form of timeout suspending.  We should consider unifying that.  We must make sure not to break bug 1167575, though.

Also, can we make suspending timeouts imply event suppression?  Today they are applied separately.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip.patch

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Some notes to jog my memory on Monday...

I'm trying to replace the current SuspendTimeouts() and Freeze() APIs with something like this:

      // Calling suspend should prevent any asynchronous tasks from
      // executing javascript for this window.  This means setTimeout,
      // requestAnimationFrame, and events should not be fired. Suspending
      // a window also suspends its children and workers.  Workers may
      // continue to perform computations in the background.  A window
      // can have Suspend() called multiple times and will only resume after
      // a matching number of Resume() calls.
      virtual void Suspend() = 0;
      virtual void Resume() = 0;
      virtual bool IsSuspended() = 0;
     
      // Calling Freeze() on a window will automatically Suspend() it.  In
      // addition, the window and its children are further treated as no longer
      // suitable for interaction with the user.  For example, it may be marked
      // non-visible, cannot be focused, etc.  All worker threads are also frozen
      // bringing them to a complete stop.  A window can have Freeze() called
      // multiple times and will only thaw after a matching number of Thaw()
      // calls.
      virtual void Freeze() = 0;
      virtual void Thaw() = 0;
      virtual bool IsFrozen() = 0; 

Suspend and Freeze state will be tracked on the inner window.  If an outer window does not have an inner window it will be considered frozen and suspended by default.

The Modal state tracking will simply trigger Suspend now.

Suspend will maintain current timer deadlines similar to how Modal does today.  Freeze will save time-to-deadline differences and Thaw will re-calculate new deadlines.  So in effect Suspend will act like time still passes, but Freeze will not.

The bfcache will use Freeze.  Pretty much everything else will use Suspend.

I'm not sure how I will handle passing a suspend count greater than one:

  https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2855
  https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8977

These are basically cases where we are attaching a window or document to a tree and want to inherit the parent state.  I may try to do a special API for this case.  I don't want people using the general API to have to reason about what count they want.  I also feel like whatever we do here should also inherit Freeze state.

Its also unclear to me if I can automatically trigger FireDelayedDOMEvents() on Thaw.  Its comment says its related to Freeze state, but that my not be accurate.  If its unrelated then I will fix the comment.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #7)
> I'm not sure how I will handle passing a suspend count greater than one:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#2855
>  
> https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#8977
> 
> These are basically cases where we are attaching a window or document to a
> tree and want to inherit the parent state.  I may try to do a special API
> for this case.  I don't want people using the general API to have to reason
> about what count they want.  I also feel like whatever we do here should
> also inherit Freeze state.

For these call sites I think I'm going to add a method like:

  virtual void
  SyncSuspendAndFreezeStateToParentWindow() = 0;

This will cause the window to look at its parent and automatically propagate the suspend and freeze counts down.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p1.patch

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p2.patch

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p3.patch

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fixes.patch

This patch set manages to run some local tests without bursting into flames.  Lets see what it does on the try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1137931f1bda

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fixes2.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7869439c7ff8

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fixes3.patch

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fixes4.patch

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p4.patch

Make alert() modal support use Suspend() instead of its own timeout pending stuff.  Does not move event suppression yet.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cadd2b7d2475

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

The integration with the ModalState is not quite ready.  It appears modal state is tracked on the scriptable top outer window, but not directly on any other window.  This doesn't play well with suspend state being tracked individually on the inner windows.

Here's a try without the p4 patch to see if there are any problems with the rest of it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ab1d9b097d

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fixes5.patch

This is basically a fix for bug 1135853.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

I also think we have a bug with how suspend/freeze work with nested workers.  See 1304489.  I'm going to leave that to the follow-up bug, though.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fixes6.patch

My "sync parent suspend state" method had incorrect logic for finding its parent.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e26c2d6c8c0

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p4.patch

Let's see if the modal state integration works now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bc7ff1e52e5

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p4.patch

When we sync the parent state we need to handle the case where our outer is in modal state, but the parent is not in modal state.  This can happen at the scriptable/chrome boundary.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ec398361149

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

The test_master_password.html test is still failing.  Oddly it thinks it can use setTimeout() during a modal:

  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_master_password.html#237

This seems... wrong.  Trying to figure out how this ever worked.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p5.patch

I believe the issue is that the test is triggering a chrome script modal dialog, but then trying to do a content script setTimeout().  This works today because the content window doesn't realize its in a modal state since IsInModalState() only looks at ScriptableTop.  If the modal window is created above scriptable top then then content window doesn't realize its in a modal.

This changes with my P4 patch because I suspend the entire window tree under the modal.

I've modified the test to use an nsITimer directly instead of setTimeout.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46274fef077d

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1303167_wip_p5.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41eb17163085

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add new window suspend and freeze methods. r=smaug

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Use new window suspend and freeze methods. r=smaug

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Use new window suspend and freeze methods. r=smaug

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove old window suspend and freeze methods. r=smaug

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ModalState use new window suspend method. r=smaug

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Fix test that attempts to use setTimeout() during modal dialog. r=smaug

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Rename new suspend and freeze methods to final names. r=smaug

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12a7225974f7916b239d6bf352f1972a010b7991&selectedJob=29804239

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add new window suspend and freeze methods. r=smaug

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Use new window suspend and freeze methods. r=smaug

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove old window suspend and freeze methods. r=smaug

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ModalState use new window suspend method. r=smaug

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Fix test that attempts to use setTimeout() during modal dialog. r=smaug

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Rename new suspend and freeze methods to final names. r=smaug

No new failures being fixed here.  Just cleaned up the API and added a couple assertions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a253aeffa1f6d82648b9f7d4ee693f9d9aab3e7

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ModalState use new window suspend method. r=smaug

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Rename new suspend and freeze methods to final names. r=smaug

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb5997ab53b504e8fcbdb8e8c32e4bc34b51f90f

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804099 [details] [diff] [review]
P1 Add new window suspend and freeze methods. r=smaug

As described in comment 0 through comment 5 there are some issues with our current window suspend, freeze, and modal code.

1) Our current SuspendTimeouts() method takes arguments determining if freezing should also occur for child entities.  This only works if ResumeTimeouts() is called in the same order guaranteeing the arguments for the last resume matches the first suspend.
2) We have multiple ways of suspending timeouts.  SuspendTimeouts() actually cancels timers.  Modal timeouts causes timers to be ignored, but refired through a separate mechanism.  Additional checks for freezing are intermixed as well.
3) We shift timers for suspend and freeze today, but not for modals.  So "time passes" for modals, but time appears not to pass for something using suspend like a sync xhr.  Arguably we should let time pass for all APIs and only shift timers for freezing in bfcache.

This patch lays out a new API:

1) nsPIDOMWindowInner::Suspend() cancels timers per our current SuspendTimeouts() mechanism, but it does not shift timers.  So time appears to pass.
2) nsPIDOMWindowInner::Freeze() automatically calls Suspend().  In addition, it marks the window as frozen indicating it should not be visible, focused, etc.
3) Outer windows without an inner are automatically considered suspended and frozen.

For convenience during refactoring I prefixed the new API with New*().  So this patch has NewSuspend(), etc.  The final patch in the bug removes this prefix.

This patch just adds the new code.  The next patches will add the calls to the code and then remove the old code.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804100 [details] [diff] [review]
P2 Use new window suspend and freeze methods. r=smaug

This patch makes the various consumers throughout the tree use the new API.

Note, I was able to remove one set of Freeze/Thaw calls since outers without an inner are automatically considered frozen and suspended.

This patch also adjust the code that maintains the timer list to understand when we need to shift time or not.  Previously we were shifting time for both suspended and frozen windows.  Now we only shift time for the frozen windows.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804101 [details] [diff] [review]
P3 Remove old window suspend and freeze methods. r=smaug

This removes the old suspend and freeze API.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804131 [details] [diff] [review]
P4 Make ModalState use new window suspend method. r=smaug

This removes our secondary method of suspending timeouts by making the EnterModalState() use the new suspend API.

Since the new API does not shift timers this is nearly identical in effect to the existing code.  The main differences are:

1) When LeaveModalState() was previously called we called all pending timers in a single runnable.  We now fire timers which may be throttled by background delays, etc.  This could change some timing (although in a spec compatible way).
2) Chrome modal windows now block content timers from running.  The current implementation does not do this because modal is tracked on the scriptable top.  So content won't see a modal tracked on a chrome window.  I believe this change should have minimal impact on real code and is worth the simplification of the code.  The next patch fixes the one test affected by this.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804103 [details] [diff] [review]
P5 Fix test that attempts to use setTimeout() during modal dialog. r=smaug

This test attempts to use a setTimeout() during a modal dialog.  After P4 that will no longer fire.  So this patch converts the test to use an nsITimer instead.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804132 [details] [diff] [review]
P6 Rename new suspend and freeze methods to final names. r=smaug

Finally, rename the new API to its final name.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Note, I also wanted to make Suspend() call SuppressEventHandling() on the window document, but I did not have time to work through that.  It became a bit thorny since mSuspendedDoc is on the outer window as far as I can tell.  It would be nice to make this work in the future, though.

Nearly every Suspend() call in the tree is paired with a SuppressEventHandling() call as well.  The one exception is ModalState() which only wants to pause animations instead of suppress all events.  I think that could be worked around by having ModalState() manually reverse the SuppressEventHandling() and then pause animations instead.

I'll file a follow-up for this before landing this bug.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8804099 [details] [diff] [review]
P1 Add new window suspend and freeze methods. r=smaug

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -1209,16 +1209,18 @@ nsGlobalWindow::nsGlobalWindow(nsGlobalW
>     mHasSeenGamepadInput(false),
> #endif
>     mNotifiedIDDestroyed(false),
>     mAllowScriptsToClose(false),
>     mTimeoutInsertionPoint(nullptr),
>     mTimeoutPublicIdCounter(1),
>     mTimeoutFiringDepth(0),
>     mTimeoutsSuspendDepth(0),
>+    mSuspendDepth(0),
>+    mFreezeDepth(0),
So mFreezeDepth should be always <= mSuspendDepth, right? Hopefully we'll have some assertions.


>+nsPIDOMWindowInner::NewSuspend()
I assume the naming is just temporary, and 'New' will be dropped later.


>+nsPIDOMWindowInner::NewSyncParentWindowState()
By just reading the method name I would expect the method to do something in parent, not sync
parent's state to 'this'. Perhaps SyncWithParentWindowState() ? or whatever is the right English for this.

>+nsGlobalWindow::NewSuspend()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(IsInnerWindow());
>+
>+  CallOnChildren(&nsGlobalWindow::NewSuspend);
>+
Could you possibly add a comment that we want to call this on children too to at least ensure that
mSuspendDepth is updated there too.


>+      // Drop the reference that the timer's closure had on this timeout, we'll
>+      // add it back in Resume(). Note that it shouldn't matter that we're
>+      // passing null for the context, since this shouldn't actually release this
>+      // timeout.
Isn't the latter sentence some leftover. We aren't passing nullon  here.



>+nsGlobalWindow::NewResume()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(IsInnerWindow());
>+
>+  CallOnChildren(&nsGlobalWindow::NewResume);
Some comment here too would be nice.

Could you somewhere here assert that mFreezeDepth == 0

>+  for (nsTimeout *t = mTimeouts.getFirst(); t; t = t->getNext()) {
Nit, nsTimeout* t

>+    int32_t remaining = 0;
>+    if (t->mWhen.IsNull()) {
I don't yet understand why this IsNull(). Based on irc this isn't needed.

>+  // Resume all of the workers for this window.  We must do this
>+  // after timeouts since workers may have queued events that can trigger
>+  // a setTimeout().
Is this really true? If so, sounds very error prone. Not all the inner windows are resumed at the same time, since 
we can be still in process of resuming others.
Followup bug?

>+nsGlobalWindow::NewFreezeInternal()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(NewIsSuspended());
>+
>+  CallOnChildren(&nsGlobalWindow::NewFreezeInternal);
>+
>+  mFreezeDepth += 1;
Could we assert here that 
mSuspendDepth >= mFreezeDepth

>+nsGlobalWindow::NewThawInternal()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(NewIsSuspended());
>+
>+  CallOnChildren(&nsGlobalWindow::NewThawInternal);
>+
>+  MOZ_ASSERT(mFreezeDepth != 0);

Could we assert here that
mSuspendDepth >= mFreezeDepth



>+nsGlobalWindow::NewIsFrozen() const
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  // No inner means we are effectively frozen
>+  if (IsOuterWindow()) {
>+    if (!mInnerWindow) {
>+      return true;
>+    }
>+    return mInnerWindow->NewIsFrozen();
>+  }
>+  bool frozen =  mFreezeDepth != 0;
extra space after =

>+nsGlobalWindow::NewSyncParentWindowState()
So this should perhaps have a bit different name too, like the one in nsPIDOMWindowInner

>+{
>+  nsPIDOMWindowOuter* outer = GetOuterWindow();
>+  if (!outer) {
>+    return;
>+  }
>+
>+  nsCOMPtr<Element> frame = outer->GetFrameElementInternal();
So, GetFrameElementInternal crosses chrome boundary. But looks like the code in nsDocShell does that too

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Interdiff 001 Address review feedback

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804409 [details] [diff] [review]
P1 Interdiff 001 Address review feedback

This address the review feedback from comment 48.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8804100 [details] [diff] [review]
P2 Use new window suspend and freeze methods. r=smaug

>   // If parent is suspended, increase suspension count.
>   // This can't be done as early as event suppression since this
>   // depends on docshell tree.
>-  if (parentSuspendCount) {
>-    privWin->SuspendTimeouts(parentSuspendCount, false);
>-  }
>+  privWinInner->NewSyncParentWindowState();
So this changes the behavior. You end up overriding privWinInner's mFreezeDepth and mSuspendDepth with parent's values.
That is not what the old code did, since parent and child may have different values. Usually here parent is chrome so it has values 0,0 but 
child might have had sync XHR running or some similar evil edge case.
So, I think NewSyncParentWindowState will need to be changed to not copy values, but merge.



>@@ -8192,17 +8192,17 @@ nsDocument::EnumerateSubDocuments(nsSubD
> bool
> nsDocument::CanSavePresentation(nsIRequest *aNewRequest)
> {
>   if (EventHandlingSuppressed()) {
>     return false;
>   }
> 
>   nsPIDOMWindowInner* win = GetInnerWindow();
>-  if (win && win->TimeoutSuspendCount()) {
>+  if (win && win->NewIsSuspended() && !win->NewIsFrozen()) {
I don't understand this change. The old code returns early whenever timeouts are suspended.
Why the new code wants also that win isn't frozen?


Why would we want to bfcache already frozen windows?
>-      // Freeze the outer window and null out the inner window so
>-      // that initializing classes on the new inner doesn't end up
>-      // reaching into the old inner window for classes etc.
>+      // The outer window is auomatically treated as frozen when we
automatically


>       // [This happens with Object.prototype when XPConnect creates
>       // a temporary global while initializing classes; the reason
>       // being that xpconnect creates the temp global w/o a parent
>       // and proto, which makes the JS engine look up classes in
>       // cx->globalObject, i.e. this outer window].
> 
>       mInnerWindow = nullptr;
> 
>-      Freeze();
>       mCreatingInnerWindow = true;
>       // Every script context we are initialized with must create a
>       // new global.
>       rv = CreateNativeGlobalForInner(cx, newInnerWindow,
>                                       aDocument->GetDocumentURI(),
>                                       aDocument->NodePrincipal(),
>                                       &newInnerGlobal,
>                                       ComputeIsSecureContext(aDocument));
>       NS_ASSERTION(NS_SUCCEEDED(rv) && newInnerGlobal &&
>                    newInnerWindow->GetWrapperPreserveColor() == newInnerGlobal,
>                    "Failed to get script global");
> 
>       mCreatingInnerWindow = false;
>       createdInnerWindow = true;
>-      Thaw();
So currently we fire the notification here. Why it is ok to not do that anymore?
Don't you end up changing EventSource and WebSocket behavior?

> HandlePrerenderingViolation(nsPIDOMWindowInner* aWindow)
> {
>   // Suspend the window and its workers, and its children too.
>-  aWindow->SuspendTimeouts();
>+  // TODO: should this be a freeze?
>+  aWindow->NewSuspend();
Most probably yes

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #51)
> Comment on attachment 8804100 [details] [diff] [review]
> P2 Use new window suspend and freeze methods. r=smaug
> 
> >   // If parent is suspended, increase suspension count.
> >   // This can't be done as early as event suppression since this
> >   // depends on docshell tree.
> >-  if (parentSuspendCount) {
> >-    privWin->SuspendTimeouts(parentSuspendCount, false);
> >-  }
> >+  privWinInner->NewSyncParentWindowState();
> So this changes the behavior. You end up overriding privWinInner's
> mFreezeDepth and mSuspendDepth with parent's values.

As you pointed, I'm wrong here and this code is fine.

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8804103 [details] [diff] [review]
P5 Fix test that attempts to use setTimeout() during modal dialog. r=smaug

I'm tiny bit worried that we need to do this change. It is possible that we'll need to revert back to the current behavior.

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8804100 [details] [diff] [review]
P2 Use new window suspend and freeze methods. r=smaug

>   nsPIDOMWindowInner* win = GetInnerWindow();
>-  if (win && win->TimeoutSuspendCount()) {
>+  if (win && win->NewIsSuspended() && !win->NewIsFrozen()) {
>     return false;
>   }
Ok, so !win->NewIsFrozen() is needed to catch the case when nsDocShell::RestoreFromHistory() re-calls
CanSavePresentation.
Please add a comment.
>-      Freeze();
>       mCreatingInnerWindow = true;
>       // Every script context we are initialized with must create a
>       // new global.
>       rv = CreateNativeGlobalForInner(cx, newInnerWindow,
>                                       aDocument->GetDocumentURI(),
>                                       aDocument->NodePrincipal(),
>                                       &newInnerGlobal,
>                                       ComputeIsSecureContext(aDocument));
>       NS_ASSERTION(NS_SUCCEEDED(rv) && newInnerGlobal &&
>                    newInnerWindow->GetWrapperPreserveColor() == newInnerGlobal,
>                    "Failed to get script global");
> 
>       mCreatingInnerWindow = false;
>       createdInnerWindow = true;
>-      Thaw();
Ok, based on IRC this is fine because this code deals with outer window only.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

> >-  if (win && win->TimeoutSuspendCount()) {
> >+  if (win && win->NewIsSuspended() && !win->NewIsFrozen()) {
> I don't understand this change. The old code returns early whenever timeouts
> are suspended.
> Why the new code wants also that win isn't frozen?

As we discussed in IRC this is necessary since nsDocShell::RestoreFromHistory() calls this with a frozen doc.  In the past this never had timers suspended, but with the new API it will always have timers suspended.  So we have to make the check more precise to let frozen windows through.

I'll add a comment to this affect.

> > HandlePrerenderingViolation(nsPIDOMWindowInner* aWindow)
> > {
> >   // Suspend the window and its workers, and its children too.
> >-  aWindow->SuspendTimeouts();
> >+  // TODO: should this be a freeze?
> >+  aWindow->NewSuspend();
> Most probably yes

I'll try making it a freeze and see what blows up.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8804100 [details] [diff] [review]
P2 Use new window suspend and freeze methods. r=smaug


>   nsPIDOMWindowInner* win = GetInnerWindow();
>-  if (win && win->TimeoutSuspendCount()) {
>+  if (win && win->NewIsSuspended() && !win->NewIsFrozen()) {
Could you still verify that this doesn't break the check that if we have for example
alert() showing on a page, that page must not enter to bfcache (pagehideevent.persisted == false)

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #56)
> >-  if (win && win->TimeoutSuspendCount()) {
> >+  if (win && win->NewIsSuspended() && !win->NewIsFrozen()) {
> Could you still verify that this doesn't break the check that if we have for
> example
> alert() showing on a page, that page must not enter to bfcache
> (pagehideevent.persisted == false)

I can try.  But showing an alert will trigger the ModalState code.  The P4 uses Suspend(), but not Freeze().  So I'm pretty confident this will still trigger the return false shortcut here.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8804131 [details] [diff] [review]
P4 Make ModalState use new window suspend method. r=smaug


>@@ -8817,114 +8817,61 @@ nsGlobalWindow::EnterModalState()
> 
>   if (topWin->mModalStateDepth == 0) {
>     NS_ASSERTION(!topWin->mSuspendedDoc, "Shouldn't have mSuspendedDoc here!");
> 
>     topWin->mSuspendedDoc = topDoc;
>     if (topDoc) {
>       topDoc->SuppressEventHandling(nsIDocument::eAnimationsOnly);
>     }
>+
>+    nsGlobalWindow *inner = topWin->GetCurrentInnerWindowInternal();
nit, nsGlobalWindow* inner

>+  MOZ_ASSERT(topWin->mModalStateDepth != 0);
>+  MOZ_ASSERT(NewIsSuspended());
>+  MOZ_ASSERT(topWin->NewIsSuspended());
>   topWin->mModalStateDepth--;
> 
>+  nsGlobalWindow *inner = topWin->GetCurrentInnerWindowInternal();
Nit, nsGlobalWindow* inner

>+  // If our outer is in a modal state, but our parent is not in a modal
>+  // state, then we must apply the suspend directly.  If our parent is
>+  // in a modal state then we should get the suspend automatically
>+  // via the parentSuspendDepth application below.
>+  if ((!parentInner || !parentInner->IsInModalState()) && IsInModalState()) {
>+    NewSuspend();
>+  }
I'd like to see some case where this is needed. Some testcase which fails without this.
Assuming you can show such and get me to look at it too, r+

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 interdiff 0001 Review feedback

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 interdiff 0001 Review feedback

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8aaf4c103d029483fee01d1d898009c10393da58

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

I will also do a try push later tonight without the check for modal state without a parent inner in SyncStateFromParenWindow().  I ran out of time right now.

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #60)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8aaf4c103d029483fee01d1d898009c10393da58

The en-US failures here were worrying me, but I see they are showing in m-c as well:

https://treeherder.mozilla.org/logviewer.html#?job_id=5392024&repo=mozilla-central#L15924

And I can reproduce locally without any of my patches applied.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Try build without the IsInModalState() check in SyncStateFromParentWindow().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf70573aaf2d667fcec3072cfdeb8b14a3ade263

Looking at this now, I think the can happen when we are replacing the inner window on the scriptable top.  It is in a modal state itself, but its parent window is not.  When the old inner is removed we losed the Suspend() it had applied for the ModalState.  We have to manually set it back in this code.

Olli, does that explanation make sense?  The try should show some blown up tests in a bit as well.

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Ben Kelly [:bkelly] from comment #63)
> Try build without the IsInModalState() check in SyncStateFromParentWindow().
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bf70573aaf2d667fcec3072cfdeb8b14a3ade263
> 
> Looking at this now, I think the can happen when we are replacing the inner
> window on the scriptable top.
Yes, that is what my idea for using alert() for the test was about.

Which test failure should I look at?

>  It is in a modal state itself, but its parent
> window is not.  When the old inner is removed we losed the Suspend() it had
> applied for the ModalState.  We have to manually set it back in this code.

Could you ensure the old inner window doesn't go to bfcache.

> 
> Olli, does that explanation make sense?  The try should show some blown up
> tests in a bit as well.
Don't yet know which test failure to look at. And I'd like to know which one breaks, so that I'm not missing some other edge case here.

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

So, one test where this matters is:

  dom/browser-element/mochitest/test_browserElement_inproc_Alert.html

I made the check an assertion temporarily and this is where it triggered:

Assertion failure: !((!parentInner || !parentInner->IsInModalState()) && IsInModalState()), at /srv/mozilla-central/dom/base/nsGlobalWindow.cpp:11944
#01: nsGlobalWindow::SyncStateFromParentWindow() (/srv/mozilla-central/dom/base/nsGlobalWindow.cpp:11944 (discriminator 1))
#02: nsPIDOMWindowInner::SyncStateFromParentWindow() (/srv/mozilla-central/dom/base/nsGlobalWindow.cpp:3892)
#03: nsGlobalWindow::SetNewDocument(nsIDocument*, nsISupports*, bool) (/srv/mozilla-central/dom/base/nsGlobalWindow.cpp:2845 (discriminator 1))
#04: nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) (/srv/mozilla-central/layout/base/nsDocumentViewer.cpp:878)
#05: nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) (/srv/mozilla-central/layout/base/nsDocumentViewer.cpp:622)
#06: nsDocShell::SetupNewViewer(nsIContentViewer*) (/srv/mozilla-central/docshell/base/nsDocShell.cpp:9342)
#07: nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) (/srv/mozilla-central/docshell/base/nsDocShell.cpp:7204)
#08: nsDocShell::CreateContentViewer(nsACString_internal const&, nsIRequest*, nsIStreamListener**) (/srv/mozilla-central/docshell/base/nsDocShell.cpp:9147)
#09: nsDSURIContentListener::DoContent(nsACString_internal const&, bool, nsIRequest*, nsIStreamListener**, bool*) (/srv/mozilla-central/docshell/base/nsDSURIContentListener.cpp:128)
#10: nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (/srv/mozilla-central/uriloader/base/nsURILoader.cpp:722)
#11: nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (/srv/mozilla-central/uriloader/base/nsURILoader.cpp:396 (discriminator 1))
#12: nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) (/srv/mozilla-central/uriloader/base/nsURILoader.cpp:259)
#13: mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*, nsISupports*) (/srv/mozilla-central/netwerk/protocol/http/HttpChannelChild.cpp:542)

If we don't have the check in SyncStateFromParenWindow() to manually apply the Suspend() we trigger this on LeaveModalState() later:

Assertion failure: IsSuspended(), at /srv/mozilla-central/dom/base/nsGlobalWindow.cpp:8847
#01: nsGlobalWindow::LeaveModalState() (/srv/mozilla-central/dom/base/nsGlobalWindow.cpp:8847 (discriminator 1))
#02: nsDOMWindowUtils::LeaveModalState() (/srv/mozilla-central/dom/base/nsDOMWindowUtils.cpp:2226)
#03: NS_InvokeByIndex (/srv/mozilla-central/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:182)
#04: CallMethodHelper::Invoke() (/srv/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2065)
#05: CallMethodHelper::Call() (/srv/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1383)
#06: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (/srv/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1350)
#07: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (/srv/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1143)
#08: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/srv/mozilla-central/js/src/jscntxtinlines.h:239)
#09: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/srv/mozilla-central/js/src/vm/Interpreter.cpp:458)
#10: InternalCall (/srv/mozilla-central/js/src/vm/Interpreter.cpp:504)
#11: js::CallFromStack(JSContext*, JS::CallArgs const&) (/srv/mozilla-central/js/src/vm/Interpreter.cpp:510)

So this handles the case where we are replacing the inner window on a top level window in a modal state.

The old inner should not enter bfcache because we still block that in CanSavePresentation() if IsSuspended() is true and the window is not already frozen.

Does all that make sense?

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Yes. That test is apparently pretty much exactly what I was thinking for this stuff.

Could you still ensure that if a page load happens during modalstate, the old page doesn't go to bfcache. As far as I see, that should be the case since nsDocument::CanSavePresentation checks it, but better to be sure.
Perhaps you could modify test_browserElement_inproc_Alert.html so that it checks that on pagehide event .persisted is false.

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

> Could you still ensure that if a page load happens during modalstate, the
> old page doesn't go to bfcache. As far as I see, that should be the case
> since nsDocument::CanSavePresentation checks it, but better to be sure.
> Perhaps you could modify test_browserElement_inproc_Alert.html so that it
> checks that on pagehide event .persisted is false.

I applied the following patch and got the output "### ### persisted false":

diff --git a/dom/browser-element/mochitest/browserElement_Alert.js b/dom/browser-element/mochitest/>
--- a/dom/browser-element/mochitest/browserElement_Alert.js
+++ b/dom/browser-element/mochitest/browserElement_Alert.js
@@ -43,20 +43,25 @@ function runTest() {
-
 function test1() {
   iframe.addEventListener('mozbrowsershowmodalprompt', test2);
-
   // Do window.alert within the iframe, then modify the global |testState|
   // after the alert.
   var script = 'data:,\
     this.testState = 0; \
+    content.onpagehide = function(evt) { sendAsyncMessage("persisted", evt.persisted); }; \
     content.alert("Hello, world!"); \
     this.testState = 1; \
   ';
-
+  mm.addMessageListener('persisted', evt => {
+    dump('### ### persisted ' + evt.data + '\n');
+  });
+
   mm.loadFrameScript(script, /* allowDelayedLoad = */ false);
-
   // Triggers a mozbrowsershowmodalprompt event, which sends us down to test2.
 }
-
 // test2 is a mozbrowsershowmodalprompt listener.
 function test2(e) {
   iframe.removeEventListener("mozbrowsershowmodalprompt", test2);

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

I will try to land this later today.

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69547514d7bd
P1 Add new window suspend and freeze methods. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d387a5da07
P2 Use new window suspend and freeze methods. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/89289670d84b
P3 Remove old window suspend and freeze methods. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4820cef74e
P4 Make ModalState use new window suspend method. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f26ccd5a281
P5 Fix test that attempts to use setTimeout() during modal dialog. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f4b01bb284
P6 Rename new suspend and freeze methods to final names. r=smaug

Comment 70

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/69547514d7bd
https://hg.mozilla.org/mozilla-central/rev/a0d387a5da07
https://hg.mozilla.org/mozilla-central/rev/89289670d84b
https://hg.mozilla.org/mozilla-central/rev/8a4820cef74e
https://hg.mozilla.org/mozilla-central/rev/7f26ccd5a281
https://hg.mozilla.org/mozilla-central/rev/e0f4b01bb284

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Aurora as well or should it ride the trains?

Comment 72

[Ryan VanderMeulen [:RyanVM]]

Got a pretty firm no to that over IRC.