1576188 - [Fission] Make Save Page As Fission-compatible

Status: VERIFIED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

Sorry for putting this in Firefox :: General, but I'm not sure where else to put it.

This bug is about making Save Page As work properly when Fission is enabled, since right now it appears to be broken.

STR:

1. Enable Fission (fission.autostart set to true, and then restart)
2. Visit any website that has cross-domain iframes (cnn.com is usually a pretty safe bet)
3. Click File > Save Page As
4. Choose some folder to save the pages to in the native file dialog
5. Click on the Downloads button to check the state of saving the page

ER:

The page should have been successfully saved when Fission is enabled.

AR:

The download is marked as "Failed"

Comment 1

[:Gijs (he/him)]

This is caused by webbrowserpersist code, which is a bunch of C++ code that lives in DOM, so I'm going to punt this into DOM.

Comment 2

[:Gijs (he/him)]

The reason this is broken is that the way things work today, the parent tells the child "please iterate over your DOM", collect subresources that need saving etc., and it does that, but in fission it cannot iterate over the subframe. It ends up propagating an NS_ERROR_NO_CONTENT to the parent and the download gets marked as failed. What'll need to happen is the parent has to walk the browsingcontext tree itself and deal with WebBrowserPersistRemoteDocuments for each of them. Each child will still have to walk its own DOM to collect other files (scripts, stylesheets, images, etc.) but we can stop dealing with subframes.

This isn't trivial to do, I suspect, and I won't have time to look at this for at least another week, and even then I doubt I'm the best person to do it. This code has very poor test coverage making it hard to ensure we don't regress anything while we're here...

Hsin-yi, bouncing the ni from bug 1590076 (which is blocked on this) back to you here in case someone else can look at this...

Comment 3

[Tim Huang[:timhuang]]

FWIW, not only 'Save Page As' breaks, but also 'Save Frame As' breaks in Fission.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Gijs for bringing this to me. I'm trying to pick up more context and background. :)

(I didn't mean to remove some dependencies, so adding them back...)

Comment 5

[Chris Peterson [:cpeterson]]

M4.1 because it blocks Security mochitests.

Comment 7

[Andreas Farre [:farre]]

Attachment: Bug 1576188 - Test that save-as works for cross process frames. r=peterv!

Comment 8

[Andreas Farre [:farre]]

Attachment: Bug 1576188 - Handle save-as for cross process iframes. r=peterv!

Depends on D70388

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ddd9a63d178
Test that save-as works for cross process frames. r=peterv
https://hg.mozilla.org/integration/autoland/rev/d2c102f8d898
Handle save-as for cross process iframes. r=peterv

Comment 10

[Arthur Iakab [arthur_iakab]]

Backed out 2 changesets (bug 1576188) for causing browser-chrome failures on browser_persist_cross_origin_iframe.js

Backout revision https://hg.mozilla.org/integration/autoland/rev/e57c48efbef3b1175bb1d6ec2322543f401b4e3d

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=d2c102f8d898b417e10342676e495fe3e6cb2117

Failure logs https://treeherder.mozilla.org/logviewer.html#?job_id=299045982&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=299032456&repo=autoland

Andreas can you please take a look?

Comment 11

[Andreas Farre [:farre]]

Yeah, so this was because of absolute paths to expected files. Turns out that it's not right to expect linux :/. Patch is update, I'll just check with peterv if he is fine with the r+

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b731cbad59a8
Test that save-as works for cross process frames. r=peterv
https://hg.mozilla.org/integration/autoland/rev/7e5e86986811
Handle save-as for cross process iframes. r=peterv

Comment 13

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1576188) for Build bustage in docshell/base/BrowsingContext.cpp. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299319089&repo=autoland&lineNumber=46513

Push with failure:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=7e5e8698681158e194dd913ec71a0fb3ba0c3e8e

Backout:
https://hg.mozilla.org/integration/autoland/rev/1b7d14f6b93e0628cc6738779a373150098ea0d5

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44ad24e311f6
Test that save-as works for cross process frames. r=peterv
https://hg.mozilla.org/integration/autoland/rev/621a1ee18070
Handle save-as for cross process iframes. r=peterv

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/44ad24e311f6
https://hg.mozilla.org/mozilla-central/rev/621a1ee18070

Comment 16

[Bogdan Maris, Desktop Test Engineering]

I was able to reproduce this using an old Nightly from 2019-08-23 on macOS 10.15, no files are downloaded and the download fails. I also see that using latest Nightly 78.0b1 the download still fails but some files are indeed downloaded, not the full content though based on the content of the html (same behavior on Windows 10 and Ubuntu 18.04 64bit). Is this expected?

Comment 17

[Andreas Farre [:farre]]

So this should be a Fission only issue, if it fails before that we've found a new bug or the test is wrong. What are trying to do that fails?

Comment 19

[Bogdan Maris, Desktop Test Engineering]

(In reply to Andreas Farre [:farre] from comment #17)

So this should be a Fission only issue, if it fails before that we've found a new bug or the test is wrong. What are trying to do that fails?

So with an affected build (that does not contain this fix) after saving the content of cnn.com (for eg) it did not download anything on local drive and inside download panel it appears that the download failed.
With a build containing this patch after saving the content of cnn.com it indeed downloaded the webpage complete files on local drive but inside download panel it again appears that the download has failed. (this happens without fission enabled as well).

So in short, the behavior is the same with or without fission enabled (files are downloaded bug inside download panel the downloads appears failed), and I guess this is treated in another bug or should be, if there is none logged.

Comment 20

[Bogdan Maris, Desktop Test Engineering]

Based on that I'll mark this as verified and will search bugzilla for a bug about the behavior explained in my previous comment.

Comment 21

[travneff]

Still actual with FF 93 - I did not enable Fission, but often have saving pages failed, because of elements blocked by noscript/ublock maybe.

Comment 22

[:Gijs (he/him)]

(In reply to travneff from comment #21)

Still actual with FF 93

No, this bug is fixed.

• I did not enable Fission,

So you're seeing a different issue.

but often have saving pages failed, because of elements blocked by noscript/ublock maybe.

This is tracked in bug 1445211.