115216 - Webbrowserpersist doesn't like mailto: links

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Dimitrios]

Build 2001121303, Win95. Url taken from bug Bug 115069. When saved as "web page
complete" not all frames are saved. No images at all and wrong frameset page.
Other framed pages are saved correctly, I cannot understand it fails on this
one. Please update summary as necessary.

Comment 1

[Dimitrios]

To [e-mail]

Comment 2

[Adam Lock]

Problems in mfcEmbed too. I'll take this one.

First impressions suggest the "/" at the front of some of the frameset URLs is
confusing something in the persist code.

Comment 3

[sairuh (rarely reading bugmail)]

i'm guessing this is prolly cross-platform. but, alas, i cannot really test Save
Frame As on linux due to bug 115154, or on mac due to bug 107521. :(

Comment 4

[Adam Lock]

I believe the bug is being triggered because nsWebBrowserPersist::Cleanup() is
being called before the DOM for the main frame and subframes has finished
traversal. This means the list of documents is wiped before they can be saved.
I'm working on a patch which defers all URI persistence until traversal has
completed so Cleanup() is not called at the wrong place.

Comment 5

[Adam Lock]

Ugh, the problem is being caused because the frames contain this line in the header:

  <link REV="made" HREF="mailto:[e-mail]">

Webbrowserpersist is failing when it can't open the mailto: channel. On MFCEmbed
this even causes it to crash because the last ref to the persist object is
released during the call out to the client nsIWebProgressListener::onStateChange.

I'm going to do what I mentioned earlier, plus trap and ignore channels that do
not do content and also put a kung-fu death grip on saveDocument to prevent
premature destruction.

Comment 6

[Adam Lock]

Attachment: Patch

This patch:

* Puts a kung-fu death grip on the object during SaveDocument
* Throws away mailto: and about: links* Adds explicit check for channels that
have no content

Reviews please

Comment 7

[Adam Lock]

Ben can you review this patch? Thanks

Comment 8

[Adam Lock]

*** Bug 116757 has been marked as a duplicate of this bug. ***

Comment 9

[Kathleen :Brade]

There are probably other protocols that should be ignored:
  javascript:
  data:

Here is a list of protocols that I've generated in the past which should be
considered for this patch:
  file, http, https, ftp,
  javascript, about, mailto, news, snews,
  ldap, ldaps, gopher, finger, telnet

Comment 10

[Boris Zbarsky [:bzbarsky]]

Also view-source:

Comment 11

[Kathleen :Brade]

and mailbox:

Comment 12

[Adam Lock]

Attachment: Work in progress

I have attached a patch that demonstrates how I intend to fix this bug, but a
dependency on bug 119496 means it won't apply cleanly so don't bother trying.
Consider it work in progress.

Basically the patch creates an C++ persist policy class that webbrowserpersist
calls to ask whether a particular URI should be saved or not. The class tests
for most of the schemes that have been listed earlier here in this bug. I
envisage this class expanding into a full-blown object with lots of methods for
bug 113902 for this bug it's just a simple class.

Comment 13

[Kathleen :Brade]

preliminary r=brade (assuming you add mailbox:)

Comment 14

[Adam Lock]

*** Bug 121585 has been marked as a duplicate of this bug. ***

Comment 15

[Adam Lock]

The checkin I made for bug 115216 last night contained a 'lite' version of this
patch which checks all the schemes mentioned above plus "irc:" but inplace
instead of in a seperate C++ policy class. That is really an issue work for bug
113902 to sort out.

Marking fixed.

Comment 16

[sairuh (rarely reading bugmail)]

over to benc to verify for the other protocol cases.

my other bugs dependent on this appear fixed.