543870 - Implement File.URL

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This will make it both easier to use file data since you don't have to use asynchronous events to load the data into memory, and will make us use much less resources since the file contents isn't read in to memory.

Note that in the current drafts the property is called File.urn. However that is expected to be changed.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP, seems to work but needs tests

Comment 2

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

Could you please use -U 8 -p when generating patches.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, this patch is not ready for review. Hence the "WIP".

Comment 4

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

yes yes, I'm just always a bit surprised to see non -U 8 -p patches.
I guess my assumption that everyone has -U 8 -p in their .hgrc is just wrong :)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I do have that, but this is the file directly from my mq queue. It seems like mq doesn't honor .hgrc.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

So this should do it. There's plenty that can be debated regarding how to do the securitymanager changes so input more than welcome.

The filechannel changes are there in order to get the XHR security checks to pass. CheckMayLoad in nsXMLHttpRequest.cpp checks both the original uri as well as the "real" uri, which doesn't work since the "real" uri returns the fileuri.

If this approach is a good one, then we might want to consider doing it for chrome and resource channels too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> It seems like mq doesn't honor .hgrc.

It does if you put this in the .hgrc:

  [diff]
  git = 1
  showfunc = true
  unified = 8

That will affect mq as well as hg export and the like.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I haven't read the details yet, but obvious questions:

1)  Why is the change to CreateCodebasePrincipal needed?
2)  It looks like various nsIURI operations on these new URIs will be broken
    (Clone() comes to mind), no?
3)  Can you actually put nsIPrincipal in netwerk/base/public?  I thought we
    couldn't.

The mHideInnerURI thing bothers me, but I don't have a better idea yet.  Having to have the "principal URI" in the uri-with-principal also bothers me.  Do we actually hit the URI-comparison code for such URIs?  In what circumstances?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #8)
> I haven't read the details yet, but obvious questions:
> 
> 1)  Why is the change to CreateCodebasePrincipal needed?

The goal is to make CheckLoadURI honor the principal in the uri. Ideally I'd like to put this code in GetCodebasePrincipal, however some of the CheckLoadURI signatures (specifically uri-uri and str-str) call CreateCodebasePrincipal rather than GetCodebasePrincipal.

Would you prefer to make those functions call GetCodebasePrincipal and put the change there instead?

> 2)  It looks like various nsIURI operations on these new URIs will be broken
>     (Clone() comes to mind), no?

Good catch. Is there any way to get this working and still use aggregation? If not it seems kind'a useless that we support aggregation on nsSimpleURI.

> 3)  Can you actually put nsIPrincipal in netwerk/base/public?  I thought we
>     couldn't.

Am I? I'm just forward declaring it there no?

> The mHideInnerURI thing bothers me, but I don't have a better idea yet.

The one thing I thought of was to change the signatures. Instead of adding hideInnerURI on nsIFileChannel, put a setOuterURI(nsIURI) function. This function sets both mOriginalURI and mHideInnerURI.

Still not clean, but slightly nicer.

> Having
> to have the "principal URI" in the uri-with-principal also bothers me.  Do we
> actually hit the URI-comparison code for such URIs?  In what circumstances?

This is there specifically to avoid using nsIPrincipal in netwerk/base/public. It's used by NS_SecurityCompareURIs in nsNetUtil.h. This is in order to get same-origin checks to check out.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1.1

Contains some test fixes, and fixes a couple of compile errors due to missing return values. Same code as previous patch otherwise.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> The goal is to make CheckLoadURI honor the principal in the uri.

Ah, I see.

> Would you prefer to make those functions call GetCodebasePrincipal and put
> the change there instead?

No, that would give wrong behavior in edge cases I bet.  Certainly making sure it doesn't would be annoying.

I'd somewhat prefer we create a new function they both call that does this and then calls on through to CreateCodebasePrincipal.  Then when we finally remove these APIs we can remove that function.

That said, I bet some of the current consumers of the principal versions of this stuff just GetCodebasePrincipal()...  So maybe we really do need this change. :(

> Is there any way to get this working and still use aggregation?

I sort of doubt it.  :(  A setup similar to nsJSURI might work, though...

> Am I? I'm just forward declaring it there no?

Historically even that has been not ok, since it wouldn't allow just-necko consumers to implement the relevant interfaces.

I talked this over with biesi, and we're ok with it in this case.

> Still not clean, but slightly nicer.

So I'm wondering... why is the XHR check checking both URIs?  It looks like we did it to be safe, but I'm not sure that's needed.  I'll try to talk it over on irc with you.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  So would it make sense to have CheckMayLoad check against the originalURI in all cases but only check against URI if LOAD_REPLACE is set on the channel?

For HTTP redirects that would be equivalent to our current code, but for chrome:// that would only check against the chrome:// URI, not the file:// one underlying it (which doesn't change much; can't load chrome://), and the same thing for your new URIs (which is what we want).

Relevant here is the behavior of NS_GetFinalChannelURI, which has exactly this sort of check.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds ok to me. Really, I should be the one asking you :)

However I have to say that I really don't like how we implement these channels in a way that results in us lying about the uri. I would much rather that we used the general approach in this patch for chrome/resource as well.

However I agree that this bug is not the place to make such a policy change.

Comment 14

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #13)
> However I have to say that I really don't like how we implement these channels
> in a way that results in us lying about the uri. I would much rather that we
> used the general approach in this patch for chrome/resource as well.
> 
> However I agree that this bug is not the place to make such a policy change.

OK.  It would be a pretty noticeably breaking change in some ways, I suspect; various things assume they can interrogate the channel URI about various properties of where it will be loaded from (e.g. whether it'll be loaded from local disk).

Do you want to update the patch, or should I try to review as-is with the caveat that you'll update it to change this part?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #11)
> > Would you prefer to make those functions call GetCodebasePrincipal and put
> > the change there instead?
> 
> No, that would give wrong behavior in edge cases I bet.  Certainly making sure
> it doesn't would be annoying.
> 
> I'd somewhat prefer we create a new function they both call that does this and
> then calls on through to CreateCodebasePrincipal.  Then when we finally remove
> these APIs we can remove that function.
> 
> That said, I bet some of the current consumers of the principal versions of
> this stuff just GetCodebasePrincipal()...  So maybe we really do need this
> change. :(

I'm having a hard time figuring out what the final verdict is here :)

Note that CreateCodebasePrincipal is not exposed outside the ssm, if that makes a difference. So outside of ssm it doesn't matter where this goes, it'll all affect GetCodebasePrincipal.

> > Is there any way to get this working and still use aggregation?
> 
> I sort of doubt it.  :(  A setup similar to nsJSURI might work, though...

Ugh, after having spent a long time on figuring out how this stuff works :( We should just remove the aggregation code then. I'll file a separate bug.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'm having a hard time figuring out what the final verdict is here :)

Leave your code as-is for now, I think.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

The whole GetURI/GetOriginalURI situation continues to scare me :(

But maybe the code that other people have written works better than my code :)

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2.1

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 429757 [details] [diff] [review]
Patch v2.1

This one uses the already existing "/dynamic/getMyDirectory.sjs" file instead of creating a new one. This makes the tests pass on windows as getMyDirectory.sjs contains fixes to account for the the '/' vs '\\' path separator problem.

Otherwise both test and code is identical to the v2 patch.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 429757 [details] [diff] [review]
Patch v2.1

> nsScriptSecurityManager::CreateCodebasePrincipal(nsIURI* aURI, nsIPrincipal **result)
>+        if (!principal || principal == mSystemPrincipal) {
>+            return CallCreateInstance(NS_NULLPRINCIPAL_CONTRACTID, result);

Why not fall back on a new nsPrincipal() in those cases?

>+        *result = principal.forget().get();

  principal.forget(result);

>+++ b/content/base/public/nsIDocument.h
>+  virtual void RegisterFileDataUri(nsACString& aUri) = 0;

Document, please.

>+++ b/content/base/src/nsDOMFile.cpp
>+nsDOMFile::GetUrl(nsAString& aURL)

>+    nsCString url = NS_LITERAL_CSTRING("moz-filedata:") +

  NS_LITERAL_CSTRING(FILEDATA_SCHEME ":") +

>+    mURL = NS_ConvertASCIItoUTF16(url);

  CopyASCIItoUTF16(url, mURL);

To actually trigger creation of a new DOMFile the user has to change what file they have selected in a form, right?  So creating thousands of these per document is not exactly likely?

>+++ b/content/base/src/nsFileDataProtocolHandler.cpp
>+ * The Initial Developer of the Original Code is
>+ * mozilla.org.

mozilla foundation, right?

>+nsFileDataURI::GetPrincipalUri(nsIURI** aUri)
>+{
>+  if (mPrincipal) {
>+    mPrincipal->GetURI(aUri);
>+  }
>+
>+  return NS_OK;

Need *aURI = nsnull if !mPrincipal.  And should probably |return mPrincipal->GtURI(aURI)| if mPrincipal.

>+nsFileDataURI::Read(nsIObjectInputStream* aStream)
>+  return aStream->ReadObject(PR_TRUE, getter_AddRefs(mPrincipal));

This may want to be NS_ReadOptionalObject.  More below.

>+nsFileDataURI::Write(nsIObjectOutputStream* aStream)
>+{
>+  nsresult rv = aStream->WriteObject(mSimpleURI, PR_TRUE);

This is assuming things about the inheritance hierarchy of mSimpleURI.  I'd rather we just use writeCompoundObject and explicitly say we want the nsIURI interface.

>+  return aStream->WriteObject(mPrincipal, PR_TRUE);

This will crash if mPrincipal is null, I think.  This shoul use NS_WriteOptionalCompoundObject.

>+nsFileDataURI::GetClassID(nsCID * *aClassID)
>+  *aClassID = (nsCID*) nsMemory::Alloc(sizeof(nsCID));
>+  return GetClassIDNoAlloc(*aClassID);

Does nsMemory::Alloc guarantee non-null return?  If not, please null-check.

>+NS_IMETHODIMP
>+nsFileDataProtocolHandler::GetProtocolFlags(PRUint32 *result)
>+{
>+  *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_SUBSUMERS;

Should this possibly have URI_NON_PERSISTABLE set?  What about URI_IS_LOCAL_RESOURCE (I'd think yes for this one)?

>+nsFileDataProtocolHandler::NewURI(const nsACString& aSpec,
>+nsFileDataProtocolHandler::NewChannel(nsIURI* uri, nsIChannel* *result)
>+  nsCOMPtr<nsIFileChannel> fileChannel = do_QueryInterface(channel);

This is no longer needed.

>+++ b/content/base/src/nsFileDataProtocolHandler.h
>+ * The Initial Developer of the Original Code is
>+ * mozilla.org.

Mozilla Foundation.

>+++ b/netwerk/base/public/nsIURIWithPrincipal.idl
>+ * The Initial Developer of the Original Code is
>+ * mozilla.org.

Mozilla Foundation.

>+    readonly attribute nsIURI principalUri;

Document how this is related to |principal|?

r=bzbarsky with the above issues fixed.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v3

This fixes all review comments, unless otherwise noted in comment above.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #20)
> (From update of attachment 429757 [details] [diff] [review])
> > nsScriptSecurityManager::CreateCodebasePrincipal(nsIURI* aURI, nsIPrincipal **result)
> >+        if (!principal || principal == mSystemPrincipal) {
> >+            return CallCreateInstance(NS_NULLPRINCIPAL_CONTRACTID, result);
> 
> Why not fall back on a new nsPrincipal() in those cases?

If we get a system principal here something has gone very wrong. Creating a codebase principal with likely a chrome uri seems very scary to me.

Similarly if we get null, we don't know what the correct principal should be. We just know that it shouldn't be the uri passed in as it implemented nsIURIWithPrincipal. Defaulting to the safest principal seems the safest to me.

> >+++ b/content/base/src/nsDOMFile.cpp
> >+nsDOMFile::GetUrl(nsAString& aURL)
> 
> >+    nsCString url = NS_LITERAL_CSTRING("moz-filedata:") +
> 
>   NS_LITERAL_CSTRING(FILEDATA_SCHEME ":") +
> 
> >+    mURL = NS_ConvertASCIItoUTF16(url);
> 
>   CopyASCIItoUTF16(url, mURL);
> 
> To actually trigger creation of a new DOMFile the user has to change what file
> they have selected in a form, right?  So creating thousands of these per
> document is not exactly likely?

That or have the user drop the file on the page using drag-n-drop. So yeah, shouldn't need to worry about vast quantities of these being created.

> >+nsFileDataURI::GetClassID(nsCID * *aClassID)
> >+  *aClassID = (nsCID*) nsMemory::Alloc(sizeof(nsCID));
> >+  return GetClassIDNoAlloc(*aClassID);
> 
> Does nsMemory::Alloc guarantee non-null return?  If not, please null-check.

I've mostly relied on infallable malloc elsewhere, but I can fix this I guess. We'll hopefully do a good sweet once infallable malloc has landed.

> >+NS_IMETHODIMP
> >+nsFileDataProtocolHandler::GetProtocolFlags(PRUint32 *result)
> >+{
> >+  *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_SUBSUMERS;
> 
> Should this possibly have URI_NON_PERSISTABLE set?  What about
> URI_IS_LOCAL_RESOURCE (I'd think yes for this one)?

Done and done.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 429868 [details] [diff] [review]
Patch v3

Oh, i suppose i should get this sr'ed given that it introduces and changes interfaces.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

http://hg.mozilla.org/mozilla-central/rev/119014ec9c62

Thanks for the reviews!

Comment 25

[Eric Shepherd [:sheppy]]

Documented here:

https://developer.mozilla.org/en/DOM/file#Properties

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. I think we should add some examples on how to use this property. Want me to add them? If so, where?

Comment 27

[Eric Shepherd [:sheppy]]

That would be awesome. How about here:

https://developer.mozilla.org/en/Using_files_from_web_applications

You slap 'em on there, I'll shuffle them around on the page to clean up organization if need be. Thanks!