Closed Bug 1056939 Opened 10 years ago Closed 10 years ago

indexedDB.open fails for certain database names

Categories

(Core :: Storage: IndexedDB, defect)

31 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: mhakobyan, Assigned: janv)

References

Details

Attachments

(4 files, 3 obsolete files)

Attached file 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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
Sorry, I actually don't know if it happens on all operating systems.
Jan's working hard to get a patch up for review by Friday but said he'll tackle this next week.
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
OS: All → Windows 8.1
Hardware: All → x86_64
I have a patch in testing:
https://tbpl.mozilla.org/?tree=Try&rev=65b51f7d9e7b

will send it for review if it passes
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c0b45ae41603
Attachment #8495783 - Flags: review?(khuey)
Attached patch patch (obsolete) — Splinter Review
Attachment #8495783 - Attachment is obsolete: true
Attachment #8495783 - Flags: review?(khuey)
Attachment #8495826 - Flags: review?(khuey)
Attached patch patch (obsolete) — Splinter Review
patch for IDB on PBackground
Attachment #8495826 - Attachment is obsolete: true
Attachment #8495826 - Flags: review?(khuey)
Attachment #8496427 - Flags: review?(khuey)
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.
Attachment #8496427 - Flags: review?(khuey) → review?(bent.mozilla)
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 on attachment 8496427 [details] [diff] [review]
patch

The test is cool though!
Attachment #8496427 - Flags: review?(bent.mozilla) → review-
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?
Attachment #8497907 - Flags: review?(Jan.Varga)
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 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 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.
Attachment #8497907 - Flags: review?(Jan.Varga) → review+
Attached patch Additional patchSplinter Review
Attachment #8496427 - Attachment is obsolete: true
Attachment #8500839 - Flags: review?(bent.mozilla)
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.
Attachment #8500839 - Flags: review?(bent.mozilla) → review+
(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 ?
Attachment #8504173 - Flags: review?(bent.mozilla)
Comment on attachment 8504173 [details] [diff] [review]
interdiff for the additional patch

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

Looks great!
Attachment #8504173 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/9c6049c49c65
https://hg.mozilla.org/mozilla-central/rev/11b19b78962a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
See Also: → 1082543
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.