407758 - Make URIS (especially file URIs) immutable before handing them back from protocol handlers

Status: RESOLVED DUPLICATE of bug 922464

(Core :: Networking, defect, P3)

Description

[Johnny Accot]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007121012 Firefox/3.0b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007121012 Firefox/3.0b2pre

In nsStandardURL, in some circumstances, it is possible to have inconsistencies between the URL spec and the path of the |file| attribute of the nsIFileURL interface.  More precisely, when the URL spec is changed, the |file| attribute isn't refreshed if it has been accessed/created already.

Reproducible: Always

Steps to Reproduce:
1. create an instance of nsStandardURL with a given spec
2. query its nsIFileURL interface
3. access the |file| attribute of nsIFileURL
4. change the path in the spec of the URL
5, print the URL spec and the |file| path
Actual Results:  
The URL spec and the |file| path are different.  The path of the |file| attribute is not updated and keeps the old path.

Expected Results:  
The path of the |file| attribute and the URL spec should always be in sync.

nsStandardURL keeps a copy of the nsIFile associated with the URL in the member variable |mFile|.  When the URL is created, |mFile| is null.  It remains null as long as |GetFile()| is not called.  When |GetFile()| is called, |mFile| is created (in nsStandardURL::EnsureFile) and reflects the URL path correctly.  But from that point, if the URL spec is changed, |mFile| is not updated and keeps the value it had when it was first created.  One solution would be to null out |mFile| each time the URL spec is changed and force it to be re-created with the new spec if |GetFile()| is called again.  The problem with this option is that the guys who called |GetFile()| before will still have stale file information.  A better solution would be to update |mFile| each time the URL spec is changed.

I will attach a short script that shows the unexpected behavior.

Comment 1

[Benjamin Smedberg]

What is the context where this happens? Nobody should be creating new nsStandardURL instances except for protocol handlers, and there's no reason I can think of for protocol handlers to access the file before they've finished initializing the URL object.

Perhaps we should assert/enforce that the URL is immutable before allowing access to the .file property.

Comment 2

[Johnny Accot]

Attachment: Show inconsistency between a File-URL spec and the associated nsIFile

Running:

xpcshell -f nsIFileURL-bug.js

yields:

uri_ok.spec: file:///foo/bar.xml; uri_ok.file.leafName: bar.xml
uri_bug.spec: file:///foo/bar.html; uri_bug.file.leafName: bar.html
uri_bug.spec: file:///foo/bar.xml; uri_bug.file.leafName: bar.html

Comment 3

[Johnny Accot]

I actually stumbled upon that issue through the |file:| protocol handler.  What I do is:  I download a file given a URL (any protocol), I do some transformation on that file, I change the file extension in the URL, and I upload the result.  What happened when I tested it with a |file:| URL is that I downloaded the file okay, I transformed it okay, I changed the extension okay, but when I uploaded the result, it just overwrote the original file.  In terms of calls, it's something like that:

nsBaseChannel::AsyncOpen ends up calling nsFileChannel::OpenContentStream (with |mUploadStream| set), which calls nsFileChannel::GetFile, which calls nsStandardURL::GetFile, which uses the old path.

So even if you think you are uploading to the target URL, you're in fact nuking your source file.  I was confused to see my source file being deleted each time.  I guess this could be considered a dangerous bug, since there can be data loss.

Comment 4

[Benjamin Smedberg]

The bug in your code is that you're calling SetSpec on a URL after it's been given to you from a URI handler. You should not do this, but rather create a new URI with the spec you desire (using the ioservice).

We should enforce this by making URIs immutable before handing them back from protocol handlers.

Comment 5

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Fixed by work in bug 922464.