nsGlobalWindow::SuspendTimeouts() is rather dubious

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(9 attachments, 24 obsolete attachments)

16.35 KB, patch
smaug
: review+
Details | Diff | Splinter Review
34.13 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.45 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.92 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.27 KB, patch
smaug
: review+
Details | Diff | Splinter Review
36.64 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.83 KB, patch
Details | Diff | Splinter Review
3.32 KB, patch
Details | Diff | Splinter Review
1.41 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
(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.
(Assignee)

Comment 3

2 years ago
The worker freezing was added for devtools in bug 1178721.
Blocks: 1178721
(Assignee)

Comment 4

2 years ago
The child freezing argument was added to SuspendTimeouts in bug 479490.
Blocks: 479490
(Assignee)

Comment 5

2 years ago
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.
See Also: → bug 1167575
(Assignee)

Updated

2 years ago
See Also: → bug 1266353
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
Created attachment 8792732 [details] [diff] [review]
bug1303167_wip_p1.patch
Attachment #8792127 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Created attachment 8792735 [details] [diff] [review]
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
(Assignee)

Comment 16

2 years ago
Created attachment 8793082 [details] [diff] [review]
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
(Assignee)

Comment 17

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1135853
(Assignee)

Comment 18

2 years ago
Created attachment 8793450 [details] [diff] [review]
fixes5.patch

This is basically a fix for bug 1135853.
(Assignee)

Comment 19

2 years ago
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.
See Also: → bug 1304489
(Assignee)

Comment 20

2 years ago
Created attachment 8793808 [details] [diff] [review]
fixes6.patch

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

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

Comment 21

2 years ago
Created attachment 8793813 [details] [diff] [review]
bug1303167_wip_p4.patch

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bc7ff1e52e5
Attachment #8793082 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
Created attachment 8793950 [details] [diff] [review]
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
Attachment #8793813 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
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.
(Assignee)

Comment 24

2 years ago
Created attachment 8794030 [details] [diff] [review]
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
(Assignee)

Comment 26

2 years ago
Created attachment 8803998 [details] [diff] [review]
P1 Add new window suspend and freeze methods. r=smaug
Attachment #8792732 - Attachment is obsolete: true
Attachment #8792733 - Attachment is obsolete: true
Attachment #8792734 - Attachment is obsolete: true
Attachment #8792735 - Attachment is obsolete: true
Attachment #8793025 - Attachment is obsolete: true
Attachment #8793080 - Attachment is obsolete: true
Attachment #8793081 - Attachment is obsolete: true
Attachment #8793450 - Attachment is obsolete: true
Attachment #8793808 - Attachment is obsolete: true
Attachment #8793950 - Attachment is obsolete: true
Attachment #8794031 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
Created attachment 8803999 [details] [diff] [review]
P2 Use new window suspend and freeze methods. r=smaug
(Assignee)

Comment 28

2 years ago
Created attachment 8804001 [details] [diff] [review]
P2 Use new window suspend and freeze methods. r=smaug
Attachment #8803999 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
Created attachment 8804002 [details] [diff] [review]
P3 Remove old window suspend and freeze methods. r=smaug
(Assignee)

Comment 30

2 years ago
Created attachment 8804003 [details] [diff] [review]
P4 Make ModalState use new window suspend method. r=smaug
(Assignee)

Comment 31

2 years ago
Created attachment 8804004 [details] [diff] [review]
P5 Fix test that attempts to use setTimeout() during modal dialog. r=smaug
(Assignee)

Comment 33

2 years ago
Created attachment 8804099 [details] [diff] [review]
P1 Add new window suspend and freeze methods. r=smaug
Attachment #8803998 - Attachment is obsolete: true
(Assignee)

Comment 34

2 years ago
Created attachment 8804100 [details] [diff] [review]
P2 Use new window suspend and freeze methods. r=smaug
Attachment #8804001 - Attachment is obsolete: true
(Assignee)

Comment 35

2 years ago
Created attachment 8804101 [details] [diff] [review]
P3 Remove old window suspend and freeze methods. r=smaug
Attachment #8804002 - Attachment is obsolete: true
(Assignee)

Comment 36

2 years ago
Created attachment 8804102 [details] [diff] [review]
P4 Make ModalState use new window suspend method. r=smaug
Attachment #8804003 - Attachment is obsolete: true
(Assignee)

Comment 37

2 years ago
Created attachment 8804103 [details] [diff] [review]
P5 Fix test that attempts to use setTimeout() during modal dialog. r=smaug
Attachment #8804004 - Attachment is obsolete: true
(Assignee)

Comment 38

2 years ago
Created attachment 8804104 [details] [diff] [review]
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
Attachment #8804005 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1311523
(Assignee)

Comment 39

2 years ago
Created attachment 8804131 [details] [diff] [review]
P4 Make ModalState use new window suspend method. r=smaug
Attachment #8804102 - Attachment is obsolete: true
(Assignee)

Comment 41

2 years ago
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.
Attachment #8804099 - Flags: review?(bugs)
(Assignee)

Comment 42

2 years ago
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.
Attachment #8804100 - Flags: review?(bugs)
(Assignee)

Comment 43

2 years ago
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.
Attachment #8804101 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8804104 - Attachment is obsolete: true
(Assignee)

Comment 44

2 years ago
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.
Attachment #8804131 - Flags: review?(bugs)
(Assignee)

Comment 45

2 years ago
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.
Attachment #8804103 - Flags: review?(bugs)
(Assignee)

Comment 46

2 years ago
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.
Attachment #8804132 - Flags: review?(bugs)
(Assignee)

Comment 47

2 years ago
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 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
Attachment #8804099 - Flags: review?(bugs) → review+
(Assignee)

Comment 49

2 years ago
Created attachment 8804409 [details] [diff] [review]
P1 Interdiff 001 Address review feedback
(Assignee)

Comment 50

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

This address the review feedback from comment 48.
Attachment #8804409 - Attachment description: hg log → P1 Interdiff 001 Address review feedback
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
Attachment #8804100 - Flags: review?(bugs) → review-
Attachment #8804101 - Flags: review?(bugs) → review+
(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 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.
Attachment #8804103 - Flags: review?(bugs) → review+
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.
Attachment #8804100 - Flags: review- → review+
(Assignee)

Comment 55

2 years ago
> >-  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 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)
(Assignee)

Comment 57

2 years ago
(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 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+
Attachment #8804131 - Flags: review?(bugs) → review+
Attachment #8804132 - Flags: review?(bugs) → review+
(Assignee)

Comment 59

2 years ago
Created attachment 8804467 [details] [diff] [review]
P2 interdiff 0001 Review feedback
(Assignee)

Comment 61

2 years ago
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.
(Assignee)

Comment 62

2 years ago
(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.
(Assignee)

Comment 63

2 years ago
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.
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
(Assignee)

Comment 65

2 years ago
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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 67

2 years ago
> 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);
(Assignee)

Comment 68

2 years ago
I will try to land this later today.

Comment 69

2 years ago
Pushed by bkelly@mozilla.com:
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
Is this something we should consider for Aurora as well or should it ride the trains?
Flags: needinfo?(bkelly)
Got a pretty firm no to that over IRC.
Flags: needinfo?(bkelly)
(Assignee)

Updated

2 years ago
Depends on: 1316289
(Assignee)

Updated

2 years ago
Depends on: 1316837
(Assignee)

Updated

2 years ago
Depends on: 1329006
(Assignee)

Updated

a year ago
Depends on: 1343874
You need to log in before you can comment on or make changes to this bug.