Closed Bug 1536530 Opened 2 years ago Closed 2 years ago

Saving a web page from Tom's Hardware forums fails

Categories

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

66 Branch
Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: jani, Assigned: smaug)

References

Details

Attachments

(1 file)

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.

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

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.

Status: UNCONFIRMED → NEW
Component: Untriaged → Downloads Panel
Ever confirmed: true
OS: Unspecified → All

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

Component: Downloads Panel → Downloads API
Flags: needinfo?(paul.boiciuc)
Product: Firefox → Toolkit

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.

Flags: needinfo?(paul.boiciuc)

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.

Component: Downloads API → DOM: Core & HTML
Product: Toolkit → Core
Priority: -- → P3

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?

Flags: needinfo?(htsai)

huh, how did webbrowserpersist end up to DOM.

Flags: needinfo?(bugs)

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

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

Kiitos Jani bugiraportista.
Tutkailen ja korjailen.

Assignee: nobody → bugs
Flags: needinfo?(bugs)

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

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2522fbc3ab9
let save-page-as work even if there are invalid urls, r=Gijs
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: needinfo?(htsai)

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

Flags: needinfo?(bugs)

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.

Flags: needinfo?(bugs)

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

Regressed by: 1101100

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

No longer regressed by: 1101100
QA Whiteboard: [good first verify]
See Also: → 1445211
You need to log in before you can comment on or make changes to this bug.