1536530 - Saving a web page from Tom's Hardware forums fails

Status: RESOLVED FIXED

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

Description

[Jani Uusitalo]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36

Steps to reproduce:

1. Open https://forums.tomshardware.com/threads/what-is-the-actually-difference-between-udimm-and-dimm.1575984/
2. Right-click and select 'Save page as'. Point the dialog to your directory of choice.

Actual results:

There's no copy of the page in the target directory. Going to about:downloads reveals that the download has failed, without further details.

Expected results:

For the directory to contain a downloaded copy of the page.

Comment 1

[Jani Uusitalo]

(My) original report on Launchpad: https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1820514

Any page under https://forums.tomshardware.com/ seems to trigger this, but I have yet to come across other sites that do. Pages can even be saved from the main Tom's Hardware domain (https://www.tomshardware.com/) without issues.

A workaround discovered by Olivier Tilloy: clicking retry in the download manager completes the save.

Comment 2

[Paul_B]

Hi reporter,

Thank you for taking the time to add this report.
I was able to reproduce the behavior that you have described on all the latest Firefox versions across OS-es ( Windows 10, Ubuntu 18.04 and Mac OS 10.14).
I'm going to add this Downloads Panel component for an advised input. If this is not the proper component, please feel free to change it to a more appropriate one.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Paul_B, can you do a quick check to see if this is a regression?

Comment 4

[Paul_B]

I tried running mozregression tool but I could not reach a regression range.
The generated builds for bisection revealed the same issue in 09/2016 / not working build on 06/2015 and a different issue on the last build 11/2014 when the page is saved but with 0 bytes.

Comment 5

[:Gijs (he/him)]

Saving only the HTML works, "web page, complete" doesn't.

There's some issue with nsIWebBrowserPersist here. There's a <script> tag on the page that has a src of //?loc=https://forums.tomshardware.com/threads/what-is-the-actually-difference-between-udimm-and-dimm.1575984/ . That's a protocol-relative URL, but you can't do that without a hostname, so creating a URL fails, which throws an NS_ERROR_MALFORMED_URI which breaks the DOM walk at https://dxr.mozilla.org/mozilla-central/rev/7f816aa10a2053973c4e6977c5d6f6bf15f38820/dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp#1101-1104 . Tom's hardware can fix this by making the URL not broken, but obviously webbrowserpersist code shouldn't fail on this.

Moving to where the source code docs say bugs in this component should go.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Gijs, thank you for diagnosing this!

I agree, webbrowserpersist should not treat NS_NewURI failures as fatal for anything other than dealing with that URI.

We could use a general "not Boris" owner for webbrowserpersist. Hsin-Yi, do you have anyone who could do that?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

huh, how did webbrowserpersist end up to DOM.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Probably because it's doing a DOM walk and should generally be kept up to date with DOM changes...

Comment 9

[:Gijs (he/him)]

https://searchfox.org/mozilla-central/rev/44a212460990ffffecf50a8e972d3cbde2e7216b/dom/webbrowserpersist/moz.build#7-9 may help in figuring out how this happened. That said, I don't know of anyone actively working on embedding who would own this, and if anything that seems like a poorer fit in terms of what the code is doing...

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Kiitos Jani bugiraportista.
Tutkailen ja korjailen.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Looks like PersistNodeFixup::FixupAnchor already just skips broken urls.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1536530, let save-page-as work even if there are invalid urls, r=gijs

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2522fbc3ab9
let save-page-as work even if there are invalid urls, r=Gijs

Comment 14

[Andrei Ciure[:aciure]]

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

Comment 15

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

Sorry for being late here. I was off the last two days. Thanks Gijs, Boris and Olli.

Comment 16

[Pascal Chevrel:pascalc]

Olli, do you think that this would be a valuable and safe uplift to beta or should we just let it ride the trains to 68? Thanks

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Given that we've had this bug at least from FF42 (bug 1101100), it is a bit hard to argue strongly why this should be taken to beta.
But, should be relatively safe.

Comment 18

[Pascal Chevrel:pascalc]

(In reply to Olli Pettay [:smaug] from comment #17)

Given that we've had this bug at least from FF42 (bug 1101100), it is a bit hard to argue strongly why this should be taken to beta.
But, should be relatively safe.

The regressing bug wasn't mentioned, so yes, with this additional context it is clearly a wontfix for 67, thanks!

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

I didn't say bug 1101100 regressed this. The code has been there at least from that bug, perhaps longer.