Unbalanced nsGlobal::Suspend/Resume for iframes

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: farre, Assigned: bkelly)

Tracking

48 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Setting a timeout from an iframe and navigating away before it fires, then going back in history to the same document where the timeout was set should fire the queued timeout.

https://farre.github.io/tests/tests/suspend/index.html
(Reporter)

Comment 1

2 years ago
(In reply to Andreas Farre [:farre] from comment #0)
> Setting a timeout from an iframe and navigating away before it fires, then
> going back in history to the same document where the timeout was set should
> fire the queued timeout.
> 
> https://farre.github.io/tests/tests/suspend/index.html

That came out a bit less clear than intended. It's not the iframe that gets navigated, but the top window. The iframe is where the timeout is set though.

Comment 2

2 years ago
[Tracking Requested - why for this release]:
This looks to be rather bad regression. 
Regression from bug 1300659 or bug 1303167.
status-firefox52: --- → affected
tracking-firefox52: --- → ?

Comment 3

2 years ago
er, that test fails everywhere for me.
status-firefox52: affected → ---
tracking-firefox52: ? → ---

Comment 4

2 years ago
Er, yes I can. I wasn't just waiting for long enough after clicking the button.

[Tracking Requested - why for this release]:
See comment 2
status-firefox52: --- → affected
tracking-firefox52: --- → ?

Comment 5

2 years ago
And the testcase is expected to fail in Chrome since it doesn't have bfcache.
(Assignee)

Comment 6

2 years ago
Fails in my nightly without bug 1300659, so I don't think it regressed from there.  Almost certainly bug 1303167.
Assignee: nobody → bkelly
Blocks: 1303167
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Blocks: 1315260
(Assignee)

Comment 7

2 years ago
When we Thaw() the parent window the docshell does not know about the frozen child frame.  So the frame is not getting thawed.
(Assignee)

Comment 8

2 years ago
Created attachment 8809059 [details] [diff] [review]
P1 Make sure we thaw child frames properly. r=smaug

This fixes it locally.  Lets see if anything else breaks:

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

I will also convert the demo site from comment 0 to a test in a P2 patch.
Tracking 52+ - as smaug notes in Comment 2 this is a rather bad regression.
tracking-firefox52: ? → +
(Assignee)

Comment 10

2 years ago
Comment on attachment 8809059 [details] [diff] [review]
P1 Make sure we thaw child frames properly. r=smaug

This patch ensures we have setup the child docshells before Thawing the window.  This is necessary for the Thaw() to propagate down to the child frame correctly.

This fixes the test case locally and passes existing tests.
Attachment #8809059 - Flags: review?(bugs)

Updated

2 years ago
Attachment #8809059 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8809451 [details] [diff] [review]
P2 Add wpt test to verify timers are restored after being loaded from bfcache. r=smaug

This patch adds a wpt test for the case demo'd in comment 0.  It passes with the P1 applied and fails without it.

I also included a test for the non-iframe case.  This passes with or without the P1 patch, but seemed reasonable to include.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a966fcfb962467482b6fffb1505d12f3f71734
Attachment #8809451 - Flags: review?(bugs)
Comment on attachment 8809451 [details] [diff] [review]
P2 Add wpt test to verify timers are restored after being loaded from bfcache. r=smaug

>+function delay(win, delay) {
>+  return new Promise(resolve => {
>+    win.setTimeout(_ => {
>+      resolve(win);
>+    }, delay);
>+  });
This won't work in browsers where data: urls are cross-origin, and we'll have that too


>+}
>+
>+function navigate_by_name(win, name) {
>+  win.location = make_post_back_url(name);
>+  return wait_for_message(name).then(_ => {
>+    return win;
>+  });
>+}
This should work since .location is special

>+
>+function go_back(win) {
>+  return new Promise(resolve => {
>+    win.onpagehide = e => resolve(win);
>+    win.history.back();
>+  });
>+}
but this shouldn't work
You'll need to use messaging (postMessage) for these.
Attachment #8809451 - Flags: review?(bugs) → review-
(Assignee)

Comment 13

2 years ago
Created attachment 8809559 [details] [diff] [review]
P2 Add wpt test to verify timers are restored after being loaded from bfcache. r=smaug

Ok.  Lets use a separate file so its same-origin, then.
Attachment #8809451 - Attachment is obsolete: true
Attachment #8809559 - Flags: review?(bugs)

Updated

2 years ago
Attachment #8809559 - Flags: review?(bugs) → review+

Comment 14

2 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbed090b39a4
P1 Make sure we thaw child frames properly. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1eb932fa31
P2 Add wpt test to verify timers are restored after being loaded from bfcache. r=smaug

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbed090b39a4
https://hg.mozilla.org/mozilla-central/rev/4d1eb932fa31
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.