1056939 - indexedDB.open fails for certain database names

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[mhakobyan]

Attachment: idbtest2.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.143 Safari/537.36

Steps to reproduce:

Create a database named: indexedDB.open('netflix.player', 1), see attached test page.


Actual results:

indexedDB.open('netflix.player', 1) results on corrupt database files, and subsequent call fails by invoking onerror.



Expected results:

Calling the database other names, for example indexedDB.open('netflix.test', 1) works fine.

It looks like the name 'netflix.player' gets hashed to "2753419432nreetyfallipx." with a trailing dot, which then gets trimmed when creating a folder... so you end up with these dir structure which is invalid.

08/20/2014  04:27 PM    <DIR>          .
08/20/2014  04:27 PM    <DIR>          ..
08/20/2014  04:27 PM    <DIR>          2753419432nreetyfallipx
08/20/2014  04:27 PM           524,288 2753419432nreetyfallipx..sqlite

Calling the daabase 'netflix.test', works fine. I assume because the file structure is created properly (file name without extension matches dir name):

08/21/2014  10:36 AM    <DIR>          .
08/21/2014  10:36 AM    <DIR>          ..
08/21/2014  10:36 AM    <DIR>          2939703975ntesteftl.ix
08/21/2014  10:36 AM           524,288 2939703975ntesteftl.ix.sqlite

Comment 1

[Jan Varga [:janv]]

Sorry, I actually don't know if it happens on all operating systems.

Comment 2

[Andrew Overholt [:overholt]]

Jan's working hard to get a patch up for review by Friday but said he'll tackle this next week.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Thanks guys!

Comment 4

[Jan Varga [:janv]]

I have a patch in testing:
https://tbpl.mozilla.org/?tree=Try&rev=65b51f7d9e7b

will send it for review if it passes

Comment 5

[Jan Varga [:janv]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=c0b45ae41603

Comment 6

[Jan Varga [:janv]]

Attachment: patch

Comment 7

[Jan Varga [:janv]]

Attachment: patch

patch for IDB on PBackground

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8496427 [details] [diff] [review]
patch

Ben is going to take this one because I'm not super-familiar with the quota manager.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8496427 [details] [diff] [review]
patch

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

I think this is a bit more complicated than it really needs to be...

You're basically enumerating the directory twice, once for potential renames and once for initializing filemanagers. Let's stick to just one enumeration:

While enumerating, just put all subdirs into 'subdirsToProcess', and add any files you encounter to 'validSubdirs' with the '.files' suffix. Once you're done enumerating just rename the directories that need it.

And we should make sure that the QuotaManager can't hand out a storage dir that ends with a '.' also...

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8496427 [details] [diff] [review]
patch

The test is cool though!

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: One directory enumeration, v1

I realized that my comment wasn't entirely good on specifics, so I went to see if I could be more clear, and then I found some problems, tinkered a little, and then... Before I knew it I had written the patch, so... What do you think?

Comment 12

[Jan Varga [:janv]]

Yeah, I wanted to do it in one step originally, but then I decided to add separate method with the extra enumeration (done only once). The extra enumeration for one time only upgrade is not so bad and code looks a bit easier to understand when you don't mix typical code path with an upgrade.

I'll look at your patch closely today.

Comment 13

[Jan Varga [:janv]]

Comment on attachment 8497907 [details] [diff] [review]
One directory enumeration, v1

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

Ok, I like your approach, I found some little problems, will send full review soon.
I want to improve the test even more since we can now recover badly created file manager directories w/o clearing entire origin.

Comment 14

[Jan Varga [:janv]]

Comment on attachment 8497907 [details] [diff] [review]
One directory enumeration, v1

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

::: dom/indexedDB/ActorsParent.cpp
@@ +9166,5 @@
> +  struct FileManagerInitInfo
> +  {
> +    nsCOMPtr<nsIFile> mDirectory;
> +    nsCOMPtr<nsIFile> mDatabaseFile;
> +  };

This doesn't compile on B2G ICS Emulator:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=382ef212e25b

Either define globally or use std::pair ?

@@ +9282,5 @@
> +    // The directory didn't have the right suffix but we might need to rename
> +    // it. Check to see if we have a database that references this directory.
> +    nsString subdirNameWithSuffix = subdirName + filesSuffix;
> +    if (!validSubdirs.GetEntry(subdirNameWithSuffix)) {
> +#ifdef XP_WIN

Hm, I think we still support sharing of profiles between different operating systems. So I guess this should be checked on all platforms.

@@ +9358,5 @@
>        // The journal file may exists even after db has been correctly opened.
>        if (NS_WARN_IF(!StringEndsWith(leafName,
>                                       NS_LITERAL_STRING(".sqlite-journal")))) {
>          return NS_ERROR_UNEXPECTED;
>        }

While we are here, I think checking for .sqlite-journal file here again is useless. We do that in the main loop, so such files should never appear in unknown files.

@@ +9361,5 @@
>          return NS_ERROR_UNEXPECTED;
>        }
>      }
>    }
>  

Unknown files should be checked after file manager initialization.

Comment 15

[Jan Varga [:janv]]

Attachment: Additional patch

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8500839 [details] [diff] [review]
Additional patch

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

::: dom/indexedDB/test/unit/test_bug1056939.js
@@ +8,5 @@
> +function testSteps()
> +{
> +  const dbName1 = "upgrade_test";
> +  const dbName2 = "testing.foobar";
> +  const dbName3 = "netflix.player";

Please rename this to something else that causes the same problem if you can...

::: dom/indexedDB/test/unit/xpcshell-head-parent-process.js
@@ +277,5 @@
> +function clearAllDatabases(callback) {
> +  resetOrClearAllDatabases(callback, true);
> +}
> +
> +function installPackagedProfile(packageName)

This doesn't seem generic enough to live here... What about just putting it in test_bug1056939.js?

::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
@@ +6,5 @@
>  dupe-manifest =
>  head = xpcshell-head-parent-process.js
>  tail =
>  support-files =
> +  bug1056939.zip

Don't forget to check this file in too.

Comment 17

[Jan Varga [:janv]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #16)
> Comment on attachment 8500839 [details] [diff] [review]
> Additional patch
> 
> Review of attachment 8500839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/test/unit/test_bug1056939.js
> @@ +8,5 @@
> > +function testSteps()
> > +{
> > +  const dbName1 = "upgrade_test";
> > +  const dbName2 = "testing.foobar";
> > +  const dbName3 = "netflix.player";
> 
> Please rename this to something else that causes the same problem if you
> can...

yeah, it can be anything looking like this: xxxxxxx.xxxxxx
or maybe even shorter

> 
> ::: dom/indexedDB/test/unit/xpcshell-head-parent-process.js
> @@ +277,5 @@
> > +function clearAllDatabases(callback) {
> > +  resetOrClearAllDatabases(callback, true);
> > +}
> > +
> > +function installPackagedProfile(packageName)
> 
> This doesn't seem generic enough to live here... What about just putting it
> in test_bug1056939.js?

I plan to use it for an upgrade that will be needed for default storage, but ok.

> 
> ::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
> @@ +6,5 @@
> >  dupe-manifest =
> >  head = xpcshell-head-parent-process.js
> >  tail =
> >  support-files =
> > +  bug1056939.zip
> 
> Don't forget to check this file in too.

Ok


one more thing, do you prefer to move |struct FileManagerInitInfo| out of the method or use std::pair ?

Comment 18

[Jan Varga [:janv]]

Attachment: interdiff for the additional patch

Comment 19

[Jan Varga [:janv]]

https://tbpl.mozilla.org/?tree=Try&rev=fbc578aca32c

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8504173 [details] [diff] [review]
interdiff for the additional patch

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

Looks great!

Comment 21

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6049c49c65
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b19b78962a

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/9c6049c49c65
https://hg.mozilla.org/mozilla-central/rev/11b19b78962a

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Mass update firefox-status to track EME uplift.