236989 - Link to stylesheet from subframe saved incorrectly if frame's parent links to the same stylesheet

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Auke]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040219
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040219

The picture in the top frame is positioned at the top. This is done via a .css
file. 

The corresponding lines in the html-file are:
<link type="text/css" href="maniakaal.css" rel="stylesheet">

and

<div id="bovenkant">
<img src="inhoud_data/inhoud.jpg" usemap="#menu" border="0">
</div>

The corresponding line in the css file is:
#bovenkant{position:absolute;top:0;left:0;}

After File -> Save Page As and opening the saved document, the css file not
handled correcly. Mozilla saves the css file on the harddisk and the
corresponding line in the new html file is:
<link type="text/css" href="Maniakaal_files/maniakaal.css" rel="stylesheet">
It could be that is wrongly saved, cause it doesn't open correctly in IE too. On
the other hand, when I open the new css file in a text editor, it looks good. It
also seems to be correctly referenced from the calling html file (as seen above).

The javascipt file is also saved on my harddisk by mozilla, but is not correctly
handled either. No menus are coming down. When opening the saved document with
IE it gives the following error:

Line 140
Character 5
Error document.GetElementById(..)filters.alpha is not an object. 
Code 0


----
When the site is saved with IE, everything works fine.



Reproducible: Always
Steps to Reproduce:
1. Go to http://maniakaal.plexus.leidenuniv.nl/
2. Choose File -> Save page as
3. Open the saved document.

Actual Results:  
* Wrong position of first paragraph/layer (<div>) of the top frame.
* In Mozilla no menu's are coming down. IE gives an error.

Expected Results:  
* Picture in top frame should be put directly against the left and top borders
of the frame.
* The menus must come down.

It could be related tot bug 120457, but since i'm not sure and because it is
only half of the problem (css problem too), I will create a new bug report.

Comment 1

[OstGote!]

Confirming Moz 1.7b 2004030909 WinNT4

see Bug 115634 for other problems

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem is that the subframe's <link> points to the same CSS file as the
parent.  So they end up with the same attribute in the saved version, but the
file is located in different places relative to them....

This is a webbrowserpersist bug.  The site has (in the toplevel document):

  <link type="text/css" href="maniakaal.css" rel="stylesheet">

  <FRAME name="boven" src="inhoud.html" scrolling=no noresize>

and in the frame

  <link type="text/css" href="maniakaal.css" rel="stylesheet">

We put the sheet as a sibling of the frame document in the hierarchy we create
but the frame and the parent have the same href attr set (which is incorrect for
the frame).

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the fundamental issue is the mRelativePathToData member in the structs in nsWebBrowserPersist.  The correct way to handle that would be to use the URI of the linking page and the URI of the data to figure out the relative path whenever it's needed....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and I'd expect this to affect images and so forth too, not just stylesheets.

Comment 6

[Priyan[:priyanmuthu]]

Hi, I would like to work on this bug. I am new to mozilla codebase and this is my first bug, please get me started on this bug.

Comment 7

[Mike Hoye [:mhoye]]

Priyan - are you still interested?

Comment 8

[Aishwarya]

Hi, I would like to work on this bug, could you help me get started?

Comment 9

[Yash Mehrotra]

Hi, I am interested in solving this bug. I have already set up my firefox codebase, but I am not able to reproduce this bug as the site mentioned in the steps seems to have changed its html from the one being discussed in comments.

Comment 10

[Mike Hoye [:mhoye]]

Hi, Yash - If you're interested, that'd be great. The bug is pretty old, though, so let's see if we can get some information about whether or not this an ongoing concern.

bz - what do you think? If this is still a thing, can we start somebody off on building a reproducible testcase?

Comment 11

[Boris Zbarsky [:bzbarsky]]

I would expect this is still a thing; I don't know of any changes to the webbrowserpersist code that would have changed things here.

Comment 12

[Mike Hoye [:mhoye]]

Ok, Yash, that means you're up to bat. I think the first thing to start on, then, is to work out a test case we can use to duplicate the results described in the first few comments. 

How does that sound, as a starting point?

Comment 13

[Yash Mehrotra]

Sounds good, I am gonna start working on it.

Comment 14

[Yash Mehrotra]

(In reply to Mike Hoye [:mhoye])

I tried and experimented a lot with html, but was not able to reproduce this bug. What do you suggest should be done?

Comment 15

[Boris Zbarsky [:bzbarsky]]

What exactly was unclear in comment 2?  ;)

Anyway, I put up a testcase for this bug at http://web.mit.edu/bzbarsky/www/testcases/document-saving/stylesheet-url-across-dirs.html just to make things clear.

Comment 16

[hristo]

Hi! I would like to have an attempt at solving this bug. The description and the test case are pretty straightforward (the bug has actually been reproduced).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Go for it!

Comment 18

[John Detloff]

Attachment: proposed-fix.patch

Comment 19

[John Detloff]

Attachment: example.zip

Comment 20

[John Detloff]

I've spent a bit of time looking at this and I think I have a pretty good picture of the issue and how it might be solved, could I get this bug assigned to me? I've attached a patch proposal and a folder that contains an example case as well as the results of saving it before and after applying the proposed patch.

The issue is a bit more general than failing to link stylesheets. I believe any linked resource that is shared between a parent and child document will save incorrectly, resulting in a bad link in either the parent or child document depending on where the child document is relative to the link node in the dom.

The example I've attached uses an iframe and an image to demonstrate the problem, and shows the link being incorrectly saved in the parent document instead of the child like the stylesheet example posted above.

My proposed solution is to keep track of the recursion depth while saving documents and always save shared data within the data dir of the shallowest document that references it. In addition, only use mPathRelativeToData on the URIData when fixing up URIs for the document to which mPathRelativeToData is relative (keep track of which document this is by storing the path on the URIData) and calculate the correct relative path when fixing up URIs for any other document.

This is my first mozilla patch, so I'd love some feedback! Posting the patch now to see if there are any objections to the general approach, I'm still reading up on the patch submission process in general - so if there's anything else I should post or individual I should ping please let me know.

Comment 21

[Boris Zbarsky [:bzbarsky]]

John, thank you for the patch, and sorry about the delayed response; I was traveling over the weekend.  And, word of warning, will be doing the same Thursday-Saturday, so responses after Wed afternoon will take a few days.

The general approach you describe sounds good, at first glance.

As far as the patch mechanics go, you probably want to create one that's using 8 lines of context instead of three in the future; those are a lot easier to review.  And I'd be happy to do the review on this; probably tomorrow.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Same patch, more context

Comment 23

[John Detloff]

Attachment: revised-fix.patch

A quick revision, updated a couple variable names and removed some unneeded whitespace changes.

Comment 24

[Boris Zbarsky [:bzbarsky]]

John, I'm sorry for the terrible lag here; I ended up being out of town for almost a week.  I'm planning to review this tomorrow.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8607936 [details] [diff] [review]
revised-fix.patch

This looks generally good, but there is one conceptual problem: FindRelativePathBetweenFileAndDirectory assumes that the file's parent directory is an ancestor of the given directory.  That happens to be the case in the example zip file, but not in this situation, I think:

Toplevel test.html:

  <iframe src="subdir1/iframe1.html"></iframe>
  <img src="subdir1/subdir2/image.jpg">

subdir1/iframe1.html:

  <iframe src="subdir2/iframe2.html"></iframe>

subdir1/subdir2/iframe2.html:

  <img src="image.jpg">

With your patch, we would want to put the mage in test_files/image.html, which means the relative URI in iframe2.html needs to become "../image.html" but FindRelativePathBetweenFileAndDirectory can't possibly produce such a URI.  Please double-check what happens in this case right now, and if I'm right that it doesn't work correctly please fix FindRelativePathBetweenFileAndDirectory to work in this situation (probably by finding the common ancestor of file and directory, figuring out how many ".." we need to get from the file's parent dir to that common ancestor, and prepending them to the string we end up creating).  If this actually works already, I'd really like to understand why it works.

The rest of the issues are pretty minor:

In the commit message,

>When fixing up uri's

should be "When fixing up URIs".

>+++ b/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp
> struct URIData
>+    uint32_t mDocumentRecurseDepth;
>+    nsCOMPtr<nsIURI> mRelativeDocumentURI;

I know this code doesn't document any of the other members of these structs (it's old, and we've gotten better about things like that since then, I hhope), but could you please document what these new members are for?  Especially mRelativeDocumentURI -- the documentation should explain which URI this is in practice.  If I read the patch right, that's the URI relative to which we'll want to make the things represented by this URIData, right?

>+nsresult nsWebBrowserPersist::FindRelativePathBetweenFileAndDirectory(nsCOMPtr<nsIFile> file, nsCOMPtr<nsIFile> directory, nsCString &relativePath)

The first two arguments should be of type "nsIFile *" here.

And please line-wrap after the commas?  That won't get you under 80 chars given how long the function name is, but it'll be close.

Also, file style is to prefix argument names with "a", so "aFile", "aDirectory", "aRelativePath".

>+    nsresult rv;

Please initialize this to NS_OK:

  nsresult rv = NS_OK;

The code as written right now will return an uninitialized value if directory is file's parent directory; initializing up front to NS_OK will avoid that.

>+        relativePathToDirectory = NS_ConvertUTF16toUTF8(dirName) + NS_LITERAL_CSTRING("/") + relativePathToDirectory;

The code that used to do this concatenation used to do it into a temporary string that it then copied into relativePathToDirectory because there are some problems with assigning a dependent concatenation containing more than two things back to one of the strings that it depends on....  Please keep doing what the code used to be doing here, with the temporary.

And please wrap this to 80 chars (e.g. by lining up the three things being concatenated, like the code used to do).

>+        rv = dirParent->GetParent(getter_AddRefs(dirParent));

The original code did GetParent into a temporary nsCOMPtr<nsIFile> on the stack, then assigned that to dirParent.  There's actually a good reason for that: last I checked C++ doesn't define the order in which the dereference operation and the function argument evaluation happen for x->y(z); compilers are free to do whatever works best for them.  But in this case function argument evaluation nulls out dirParent, so if the argument is evaluated before the dereference this code will crash.  Please use a newDataDirParent temporary like the old code did.

>@@ -3381,17 +3392,40 @@ nsWebBrowserPersist::FixupURI(nsAString &aURI)
>+        nsAutoCString rawPathURL(relativePath);

Why not just declare rawPathURL before the new if statement like so:

  nsAutoCString rawPathURL;

and then in one branch of the if do:

  rawPathURL = data->mRelativePathToData;

and in the other branch do:

  ....
  rv = FindRelativePathBetweenFileAndDirectory(docFile, parentDir, rawPathURL);

?  That is, why do we need the relativePath temporary?

>@@ -3740,16 +3774,23 @@ nsWebBrowserPersist::MakeAndStoreLocalFilenameInURIMap(
>+        if (data->mDocumentRecurseDepth > mCurrentDocumentRecurseDepth) {

This could use a comment about how we're closer to the root than the document that was used for "data"'s relative path, so we want to store the data in our data dir, not that document's.

Again, sorry this took so long to review; it took me a while to re-familiarize myself with this code and travel interfered.  At this point I have a much better idea of what's going on here, so following reviews should be much faster.

Comment 26

[John Detloff]

Attachment: Thanks for the review! Here's a revised diff that I think addresses all your comments.

Comment 27

[John Detloff]

Attachment: fix-after-review.patch

Woops, quick correction to that last diff

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8614545 [details] [diff] [review]
fix-after-review.patch

>the URIData is relative to. When fixing up uri's for this document,
>use that mRelativePathToData. When fixing up URI's for any other

"URIs", not "uri's" or "URI's", on both of those lines.

>+++ b/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp
>+    // URI of the document mRelativePathToData is relative from

"relative to", I'd think.

I don't think FindRelativePathBetweenAncestorAndDescendent does what we want it to.  Just because mRelativeDocumentURI is not the same as mTargetBaseURI doesn't mean the document is a descendant of the parent dir of the image.  Consider, for example, this case:

test.html:

  <iframe src="subdir1/iframe1.html"></iframe>
  <iframe src="subdir2/iframe2.html"></iframe>

subdir1/iframe1.html:

   <img src="test.jpg">

subdir2/iframe2.html:

   <img src="../subdir1/test.jpg">

I'm pretty sure this won't work with the function as written.

Comment 29

[John Detloff]

Ah, I see what you mean, you're right we cannot rely on the data dir always being an ancestor of the linking document. Thanks for catching that, let me just attach another swing at this fix

Comment 30

[John Detloff]

Attachment: fix-after-review.patch

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8621458 [details] [diff] [review]
fix-after-review.patch

John, have you tested this with non-ASCII paths, especially on a non-UTF-8 system?  GetRelativeDescriptor seems likely to return the string in the native filesystem encoding, but then it's being appended to a URL, where it will get treated as UTF-8....

Comment 32

[Boris Zbarsky [:bzbarsky]]

Though I guess the actual implementation of getRelativeDescriptor does return UTF-8 at the moment.  It's just the interface that says to expect anything.

Benjamin, can we just assume that getRelativeDescriptor returns a UTF-8 path?

Comment 33

[Benjamin Smedberg]

The docs don't support that assumption currently, and it wasn't true in the past (Mac relative descriptors used to be opaque strings). Nowadays the actual implementation seems to promise that: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileCommon.cpp#263

So perhaps it's safe to change the docs and make that promise. But froydnj should make that decision please.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Nathan, see comment 33?

Comment 35

[Nathan Froyd [:froydnj]]

I think what Benjamin is referring to in comment 33 is aliases:

https://en.wikipedia.org/wiki/Alias_%28Mac_OS%29

which are similar to symbolic links (Unix) or shortcuts (Windows).  It looks like SetRelativeDescriptor still handles them (do we still need to do this?  that code looks gnarly):

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1913

but it winds up normalizing them into actual file paths, which are what get stored in nsLocalFileUnix, and what GetRelativeDescriptor winds up dealing with.

So I think we can change the documentation to say we're always going to return UTF-8 without any ill effects.

Comment 36

[Boris Zbarsky [:bzbarsky]]

OK, excellent.  I filed bug 1175600 on that.

John, why can't we just use getRelativeDescriptor in FindRelativePathFromAncestorToDescendent as well?

Comment 37

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I just checked in nsIFile::GetRelativePath in bug 1175600 which guarantees a UTF-8 path per contract and which we should use here instead of GetRelativeDescriptor.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8621458 [details] [diff] [review]
fix-after-review.patch

Please see comment 36.

Comment 39

[John Detloff]

Sorry for the delay, here's another swing at this that utilizes GetRelativePath and works with the new asynch logic in this class

Comment 40

[John Detloff]

Attachment: updated_fix.patch

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8656417 [details] [diff] [review]
updated_fix.patch

The nice commit message went away.  :(

This looks like it rebuilds mFlatURIMap on every call to SerializeNextFile.  Why is that OK, or desirable?

The rest looks good, I think, but I'd like to understand what's going on with mFlatURIMap.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Or I guess the point is that mFlatURIMap needs to be redone every time we start serializing a new document because we have a new base URI so need to change our mappings?  That should be documented clearly.

Oh, one more thing.  The order of the two arguments to GetLocalURI should probably be switched so the outparam is the last argument.

Comment 43

[John Detloff]

Attachment: updated_fix.patch

Yep! Since each document being serialized can have a different location the relative links have to be regenerated to ensure they work from the new document.

Added a bit of documentation, reordered the params in GetLocalURI, and fleshed out the commit message.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8657643 [details] [diff] [review]
updated_fix.patch

>+    // mFlatURIMap must be rebuilt with each time through SerializeNextFile, as mTargetBaseURI is

s/with each time/each time/

>+    // used to create the relative URL's and will be different with each serialized document.

s/URL's/URLs/

r=me

Comment 45

[Boris Zbarsky [:bzbarsky]]

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=eae07b417904 looks pretty reasonable....

Comment 46

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with those comment fixes and some noise removed from the commit message and bug number added

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/849943de5801

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/849943de5801

Comment 49

[Boris Zbarsky [:bzbarsky]]

John, thank you again for fixing this, and for sticking with it through the whole process!

Comment 50

[John Detloff]

No prob, thanks for the help!