Closed Bug 1597499 Opened 5 years ago Closed 4 years ago

[Fission] Fix session restore to work for cross-origin iframes

Categories

(Firefox :: Session Restore, defect, P1)

Product:
Firefox
Component:
Session Restore
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
89 Branch
Project Flags:
Fission Milestone M7a
Tracking Flags:
Tracking Status
firefox89 --- fixed

People

(Reporter: djvj, Assigned: u608768)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [rm-docshell-tree-item:hard])

Attachments

(4 files, 5 obsolete files)

Bug 1597499 - Move nsIDocShell::ChildOffset to BrowsingContext, r?nika
(deleted), text/x-phabricator-request
Details
Bug 1597499 - Make Session Restore work in Fission, r?nika
(deleted), text/x-phabricator-request
Details
Bug 1597499 - Enable some sessionstore Fission/SHIP tests, r?farre
(deleted), text/x-phabricator-request
Details
Bug 1597499 - Add SessionStoreTypes.ipdlh, r?farre
(deleted), text/x-phabricator-request
Details

In file toolkit/components/sessionstore/SessionStoreUtils.cpp

Called from RestoreFrameTreeData in session-store code.

Iterates over children of docshell, checking for CreatedDynamically flag and skipping those children.

For non-skipped children, calls a callback which can run arbitrary script, passing the window-proxy for the child.

Change to use BrowsingContext to traverse list of children.

For out-of-process children, do whatever fancy fission stuff is needed to obtain a WindowProxy to a remote window, and then pass that.

This one is likely a bunch of work.

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Fission Milestone: --- → M6
Priority: -- → P3

Alphan, are you the right contact for Fission Session Store questions? If so, can you please audit this use of the nsIDocShellTreeItem interface in SessionStoreUtils.cpp?

With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null.

If this code is broken with Fission, fixing it blocks enabling Fission is Nightly.

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Component: DOM: Navigation → Session Restore
Flags: needinfo?(alchen)
Priority: P3 → --
Product: Core → Firefox

Yes, I will take care of this bug.

Assignee: nobody → alchen
Flags: needinfo?(alchen)
Priority: -- → P2
Fission Milestone: M6 → M6c

The severity field is not set for this bug.
:mikedeboer, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mdeboer)

Severity S3 because this problem only affects Fission users.

Severity: normal → S3
Flags: needinfo?(mdeboer)
Attachment #9142672 - Attachment is obsolete: true
Assignee: alchen → kmadan
Status: NEW → ASSIGNED

We don't collect form data or scroll positions for out-of-process frames, so we can't really test if we're fixing anything here. Blocking on bug 1572084.

Depends on: 1572084
Fission Milestone: M6c → M6b
Whiteboard: [rm-docshell-tree-item:hard] → ETA: 9/11 [rm-docshell-tree-item:hard]

Depends on D89224

Attachment #9162435 - Attachment is obsolete: true
Attachment #9175295 - Attachment description: Bug 1597499 - Move nsIDocShell::ChildOffset to BrowsingContext, r?peterv → Bug 1597499 - Move nsIDocShell::ChildOffset to BrowsingContext, r?nika
Attachment #9175296 - Attachment description: Bug 1597499 - Use a JSProcessActor to replace the usage of SessionStoreUtils::ForEachNonDynamicChildFrame, r?peterv → Bug 1597499 - Use a JSProcessActor to replace the usage of SessionStoreUtils::ForEachNonDynamicChildFrame, r?nika
Whiteboard: ETA: 9/11 [rm-docshell-tree-item:hard] → patches in review [rm-docshell-tree-item:hard]

Discussed with Nika and Kashav:
This doesn't necessarily block the Nightly experiment and this needs some more time to be implemented correctly.

Fission Milestone: M6b → M6c
Whiteboard: patches in review [rm-docshell-tree-item:hard] → [rm-docshell-tree-item:hard]
Blocks: 1674871
Fission Milestone: M6c → M7

Renaming the bug to indicate it is a bigger redesign - restore counterpart of Bug 1572084.

Summary: Fix uses of mozilla::dom::SessionStoreUtils::ForEachNonDynamicChildFrame in toolkit/components/sessionstore/SessionStoreUtils.cpp → [Fission] Fix session restore to work for cross-origin iframes

Depends on D89969

Depends on D107882

Attachment #9175296 - Attachment is obsolete: true
Depends on: 1697645
Blocks: 1698104

We compare these against the nsIRequest's URI to decide if we should send a
SSTabRestored, so they should be fixed-up.

Depends on D107882

Attachment #9208902 - Attachment is obsolete: true
Blocks: 1698601
Priority: P2 → P1
Blocks: 1698878
Attachment #9208189 - Attachment is obsolete: true

Originally authored by :farre for bug 1572084

Depends on D89969

Attachment #9209562 - Attachment description: Bug 1597499 - Enable sessionstore Fission/SHIP tests, r?farre → Bug 1597499 - Enable some sessionstore Fission/SHIP tests, r?farre
Blocks: 1699177
Pushed by kmadan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/12762a25c4fa Move nsIDocShell::ChildOffset to BrowsingContext, r=nika https://hg.mozilla.org/integration/autoland/rev/e84054ccadb0 Add SessionStoreTypes.ipdlh, r=farre https://hg.mozilla.org/integration/autoland/rev/d015ba9097c5 Make Session Restore work in Fission, r=nika https://hg.mozilla.org/integration/autoland/rev/0df7b8660446 Enable some sessionstore Fission/SHIP tests, r=farre

Backed out for causing Fission crashes (bug 1699349)

Backout link: https://hg.mozilla.org/mozilla-central/rev/8076082c6ed9dfac47115fe1766ebab510149acb

Flags: needinfo?(kmadan)
Status: RESOLVED → REOPENED
status-firefox88: fixed → ---
Resolution: FIXED → ---
Target Milestone: 88 Branch → ---

Looking into it, sounds like we're trying to restore data into a discarded browsing context. Sorry for the noise.

Flags: needinfo?(kmadan)
Fission Milestone: M7 → M7a
Pushed by kmadan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e618ac693605 Move nsIDocShell::ChildOffset to BrowsingContext, r=nika https://hg.mozilla.org/integration/autoland/rev/1f30fb9d9356 Add SessionStoreTypes.ipdlh, r=farre https://hg.mozilla.org/integration/autoland/rev/0a97bfe2a112 Make Session Restore work in Fission, r=nika https://hg.mozilla.org/integration/autoland/rev/b6ec8682e6dc Enable some sessionstore Fission/SHIP tests, r=farre
Blocks: 1648158
No longer depends on: 1687495
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: