Closed Bug 1576188 Opened 1 year ago Closed 3 months ago

[Fission] Make Save Page As Fission-compatible

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla77
Fission Milestone M4.1
Tracking Status
firefox77 --- verified

People

(Reporter: mconley, Assigned: farre)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

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"

Priority: -- → P3
No longer blocks: 1575386
See Also: → 1575386

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.

Blocks: fission
No longer blocks: fission-frontend
Component: General → DOM: Core & HTML
Priority: P3 → --
Product: Firefox → Core
Blocks: 1590076

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...

Flags: needinfo?(htsai)
Fission Milestone: --- → ?
Flags: needinfo?(htsai)
Priority: -- → P3

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

Blocks: 1586726

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...)

No longer blocks: 1586726
Blocks: 1586726

M4.1 because it blocks Security mochitests.

Fission Milestone: ? → M4.1
Assignee: nobody → afarre
Status: NEW → ASSIGNED
Priority: P3 → P2
Depends on: 1599616
Duplicate of this bug: 1599616
Pushed by afarre@mozilla.com:
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

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+

Flags: needinfo?(afarre)
Pushed by afarre@mozilla.com:
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
Pushed by afarre@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Flags: needinfo?(afarre)
Flags: qe-verify+
Regressions: 1636903

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?

Flags: needinfo?(afarre)

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?

Flags: needinfo?(afarre) → needinfo?(bogdan.maris)
Duplicate of this bug: 1590076

(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.

Flags: needinfo?(bogdan.maris)

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.