1321550 - indexedDB includes fragment identifiers in file:// URLs as hostnames

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Creating an indexedDB under a file:/// path includes everything after a # for a domain. So going to the following URLs and creating a DB will create three completely separate DBs:

  - file:///some/path/test.html
  - file:///some/path/test.html#test
  - file:///some/path/test.html#/test

The DBs are created at:

  - file++++some+path+test.html
  - file++++some+path+test.html#test
  - file++++some+path+test.html#+test

Chrome and Safari instead group all file:/// indexedDBs into one location with a hostname of file__0. I think our approach is better but surely we should trim the host at the first #?

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I need to know what we are supposed to do here so that I can display the correct DB info in the Storage Inspector.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This is controlled by QuotaManager and not really specific to IndexedDB.  It may be related to how we treat the origin for file URLs in general.  Not sure what our principal code does there.  Jan, what do you think?

Comment 3

[Jan Varga [:janv]]

Yeah, this goes through QuotaManager, but we just use what nsIPrincipal::GetOrigin() gives us. We don't do anything special for file:// in QuotaManager.
I'm not an expert on file protocol, there can be a reason why it's treated this way.
We have an IndexedDB bug somewhere about file:// origins being too long.

Comment 4

[Andrea Marchesini [:baku]]

I'll take a look at this. It seems that we are retrieving the origin wrongly for file URLs.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: filepath.patch

Comment 6

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

Comment on attachment 8821149 [details] [diff] [review]
filepath.patch

nsIURIWithQuery is so mysterious that I don't know when one is supposed to use it. I have no idea if this is right, or whether same kind of check should be done with some other schemes.
(Initial reaction is that adding explicit SchemeIs check is wrong.)

Can't really review this.

Comment 7

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

So if this patch was taken (though it is confusing to reuse hostPort for filePath), which all schemes would still fallback to 
nsCOMPtr<nsIStandardURL> standardURL = do_QueryInterface(origin);
NS_ENSURE_TRUE(standardURL, NS_ERROR_FAILURE);
rv = origin->GetAsciiSpec(aOrigin);

?

Comment 8

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

Do we possibly want to add GetAsciiSpecIgnoringRef and use it there and not GetAsciiSpec?

Comment 9

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Do we possibly want to add GetAsciiSpecIgnoringRef and use it there and not
> GetAsciiSpec?

This would also be a nice approach. I prefer valentin to take a decision here.

Comment 10

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

Yes, GetAsciiSpecIgnoringRef seems to be what we want here.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: filepath2.patch

Comment 12

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

Comment on attachment 8821713 [details] [diff] [review]
filepath2.patch

Review of attachment 8821713 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsPrincipal.cpp
@@ +182,5 @@
>    // of the spec, and the beginning of the origin attributes) is not present
>    // in the origin string
>    nsCOMPtr<nsIStandardURL> standardURL = do_QueryInterface(origin);
>    NS_ENSURE_TRUE(standardURL, NS_ERROR_FAILURE);
> +  rv = origin->GetAsciiSpecIgnoringRef(aOrigin);

Sorry for changing my mind about this, but after seeing the patch, it seems we're adding a lot of complexity for nothing, even as bug 945240 will eventually fix this.
Anyway, I thing GetAsciiSpec and then truncating at the first `#` should do the same thing, with more or less the same overhead.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: filepath3.patch

Comment 14

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

Comment on attachment 8821734 [details] [diff] [review]
filepath3.patch

Review of attachment 8821734 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsPrincipal.cpp
@@ +189,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  int32_t pos = spec.FindChar('?');

nit: add a comment mentioning why we are doing this.

@@ +190,5 @@
>  
> +  int32_t pos = spec.FindChar('?');
> +  int32_t hashPos = spec.FindChar('#');
> +
> +  if (pos != kNotFound && hashPos != kNotFound && hashPos < pos) {

bug: if pos==kNotFound, we never enter this branch.

Please add tests for this. We need at least 4.
file:///path?query#hash
file:///path#hash
file:///path?query
file:///path#hash?query

@@ +194,5 @@
> +  if (pos != kNotFound && hashPos != kNotFound && hashPos < pos) {
> +    pos = hashPos;
> +  }
> +
> +  if (!aOrigin.Assign(spec, pos != kNotFound ? pos : spec.Length(), fallible)) {

nit: the ternary conditional makes this harder to read.
I would have gone for GetAsciiSpec(aOrigin); find cutoff point. if (pos != kNotFound) aOrigin.Truncate(pos);

Comment 15

[Andrea Marchesini [:baku]]

Attachment: filepath3.patch

Is there an easy way to write tests for file URL?

Comment 16

[Andrea Marchesini [:baku]]

Attachment: filepath3.patch

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Jan, do we need some kind of migration or cleanup for origins on disk for our old serialization?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/974a431df67c
origin should not contain ref part of the URL, r=valentin

Comment 19

[Jan Varga [:janv]]

(In reply to Ben Kelly [:bkelly] from comment #17)
> Jan, do we need some kind of migration or cleanup for origins on disk for
> our old serialization?

So before this patch we created and used separate origin directories, for example:
file++++some+path+test.html
file++++some+path+test.html#test
file++++some+path+test.html#+test
 
After the patch, only the first one will be used/shared.
So the 2nd and 3rd one won't be ever used again. It would be nice to delete them immediately, but I'm not sure it's worth to do it given that file:// is not used much by typical users. Anyway the unused directories should get removed once the global limit for default/temporary storage is reached.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/974a431df67c