1668414 - Saving a wikipedia page is missing images used from the page (because srcset attribute is not scanned/re-written)

Status: RESOLVED FIXED

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

Description

[Digi]

Attachment: Mozilla.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

Open a page, right click, "Save page as..."

Actual results:

The list of downloads shows an error. Some page files are saved, some are not.

Expected results:

Page is saved.

Comment 1

[Alice0775 White]

I can reproduce the issue on Nightly83.0a1 Windows10. Chrome works without failure.

1. Open https://developer.mypurecloud.com/forum/t/problem-with-the-transcription-of-an-interaction/8507
2. Click Accept button if Cookie Consent dialog pops up.
3. Save page as(Ctrl+S) > Web page complete > Save

Comment 2

[Marco Bonardo [:mak]]

it fails everytime on Save Page As, but it works when retrying the download from the panel. That sounds like some principal/security failure...

Comment 3

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #1)

I can reproduce the issue on Nightly83.0a1 Windows10. Chrome works without failure.

1. Open https://developer.mypurecloud.com/forum/t/problem-with-the-transcription-of-an-interaction/8507
2. Click Accept button if Cookie Consent dialog pops up.
3. Save page as(Ctrl+S) > Web page complete > Save

This looks to me like it's due to mixed content blocking. Can you file a separate bug? Although the result (download fails) is the same, I think the cause is different.

Comment 4

[:Gijs (he/him)]

Can you attach your about:support information? I'd be particularly interested if you have something like uBlock origin or another ad or content blocker installed, or if you're using tracking protection (did you save the page from a private browsing window) ?

As it is, saving the mozilla homepage (which seems to be what you tried to do in comment #0, based on the screenshot) works for me in 82 beta on a relatively clean profile - but there are known issues with the page saving code when ad/tracking-blocking blocks some of the requests for parts of the page (bug 1445211).

Comment 5

[Digi]

(In reply to :Gijs (he/him) from comment #4)

Can you attach your about:support information? I'd be particularly interested if you have something like uBlock origin or another ad or content blocker installed, or if you're using tracking protection (did you save the page from a private browsing window) ?

As it is, saving the mozilla homepage (which seems to be what you tried to do in comment #0, based on the screenshot) works for me in 82 beta on a relatively clean profile - but there are known issues with the page saving code when ad/tracking-blocking blocks some of the requests for parts of the page (bug 1445211).

I have uBlock origin, and disabling it seems to solve the problem in most cases indeed. However, some pages still fail in one way or another.
For instance, saving https://en.wikipedia.org/wiki/Belousov%E2%80%93Zhabotinsky_reaction results in gif animations not shown in the saved page.

Comment 6

[:Gijs (he/him)]

(In reply to Digi from comment #5)

For instance, saving https://en.wikipedia.org/wiki/Belousov%E2%80%93Zhabotinsky_reaction results in gif animations not shown in the saved page.

Let's morph to address this issue, then, as the uBlock issue is covered elsewhere.

The webbrowserpersist code, which walks the DOM and finds resources that need saving, would seem to be responsible for this, so I'm moving this over to the relevant component and adjusting the summary.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

The issue is that we're not rewriting srcset urls. Should be easily fixable.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(The fix was actually relatively straight-forward, but as it turns out there are close to zero tests for this, so I'm adding some)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1668414 - Miscellaneous ResponsiveImageSelector cleanups. r=edgar

Enum classes and a couple other simplifications.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1668414 - Handle srcset in WebBrowserPersist. r=edgar

Rewrite the srcset URIs appropriately...

This code was completely untested (modulo one test for forms...) I added a
generic reftest framework to compare original vs. persisted document, so that
testing changes to this code is easier next time.

Depends on D92132

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c30ee90cac6a
Miscellaneous ResponsiveImageSelector cleanups. r=edgar

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c30ee90cac6a

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1668414 - Also handle <source srcset>. r=edgar

Thanks for asking me to add a test ;)

Using the image responsive selector works for scanning the srcset images, but
since we wouldn't rewrite the <source> uris the invalid urls would still have
preference.

Depends on D92133

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cef0f66ca3e7
Handle srcset in WebBrowserPersist. r=edgar
https://hg.mozilla.org/integration/autoland/rev/94a576dd5f89
Also handle <source srcset>. r=edgar

Comment 15

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1668414) for causing build bustages in WebBrowserPersistLocalDocument.cpp CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cx64%2Cdebug%2Cbuild-linux64-base-toolchains%2Fdebug%2Cbb&revision=94a576dd5f89d0e10db0c7e8b017bbcc94bc4998

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=317603861&repo=autoland&lineNumber=80398

Backout: https://hg.mozilla.org/integration/autoland/rev/892c085e3cba1bdd4169d415700ef3517cdcf248

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8a22668fbef
Handle srcset in WebBrowserPersist. r=edgar
https://hg.mozilla.org/integration/autoland/rev/effdabb1dfd7
Also handle <source srcset>. r=edgar

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b8a22668fbef
https://hg.mozilla.org/mozilla-central/rev/effdabb1dfd7

Comment 18

[:Gijs (he/him)]

I think this might be fixed based on these last commits?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91464994c8ac
Restore binary file that was empty when re-landing due to bug 1709608.

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/91464994c8ac