Last Comment Bug 765839 - Enable OOP test suite
: Enable OOP test suite
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
: Hsin-Yi Tsai [:hsinyi]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 11:05 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2012-08-17 03:22 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (64.47 KB, patch)
2012-06-18 11:05 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-18 11:05:44 PDT
Created attachment 634114 [details] [diff] [review]
Patch, v1

We need to ensure that all the indexedDB tests work in a child process. This patch fixes all the races that were unearthed by try and should help us identify anything that breaks OOP indexedDB.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-19 13:59:03 PDT
Comment on attachment 634114 [details] [diff] [review]
Patch, v1

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

/me goes to shower

::: build/automationutils.py
@@ +107,5 @@
>  
>  def isURL(thing):
>    """Return True if |thing| looks like a URL."""
>    # We want to download URLs like http://... but not Windows paths like c:\...
> +  return thing and len(urlparse(thing).scheme) >= 2

Ditch this.

::: dom/indexedDB/IDBTransaction.cpp
@@ +249,5 @@
>    if (!IndexedDatabaseManager::IsMainProcess()) {
>      NS_ASSERTION(mActorChild, "Must have an actor!");
>  
>      mActorChild->SendAllRequestsFinished();
> +      //PR_Sleep(PR_SecondsToInterval(2));

Remove this.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +601,5 @@
> +    // from the list if necessary.
> +    if (op->mDatabases.RemoveElement(aDatabase)) {
> +      // Now set up the helper if there are no more live databases.
> +      NS_ASSERTION(op->mHelper, "How did we get rid of the helper before "
> +                    "removing the last database?");

op->mHelper || op->mRunnable is what you want here.

@@ +1098,3 @@
>    }
>  
> +  PRUint32 runCount = service && pool && !databases.IsEmpty() ? 2 : 1;

This is pretty awful.  At least name this waitCount instead of runCount.

@@ +1135,4 @@
>  }
>  
>  bool
> +IndexedDatabaseManager::IsClearOriginPending(const nsACString& aOrigin)

This should just wrap FindSynchronizedOp(aOrigin, null).

@@ +1438,5 @@
> +      // Remove the directory that contains all our databases.
> +      nsCOMPtr<nsIFile> directory;
> +      nsresult rv =
> +        mgr->GetDirectoryForOrigin(mOrigin, getter_AddRefs(directory));
> +      NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Fauled to get directory to remove!");

*Failed

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +90,2 @@
>                                    aCallback, aClosure);
>    }

These changes can be can reverted.

@@ +220,5 @@
>    void OnDatabaseClosed(IDBDatabase* aDatabase);
>  
>    // Responsible for clearing the database files for a particular origin on the
>    // IO thread. Created when nsIIDBIndexedDatabaseManager::ClearDatabasesForURI
> +  // is called. Runs four times, twice on the main thread, then once on the IO

Revert this comment.

@@ +269,5 @@
> +      }
> +    }
> +
> +    static void ExclusiveCallback(nsTArray<nsRefPtr<IDBDatabase> >& aDatabases,
> +                                  void* aClosure);

This needs a better name.  One that doesn't imply that you already have exclusive access ... how about "InvalidateOpenedDatabasesCallback"?

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1891,5 @@
>  
>    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
>    NS_ASSERTION(mgr, "This should never be null!");
>  
> +  rv = mgr->AcquireExclusiveAccess(mDatabase, mDatabase->Origin(), helper,

Revert.

@@ +1920,5 @@
>  
>    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
>    NS_ASSERTION(mgr, "This should never be null!");
>  
> +  rv = mgr->AcquireExclusiveAccess(mDatabase, mDatabase->Origin(), helper,

Revert.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-19 18:52:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/30bfeaf1e175
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-19 21:12:58 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> @@ +90,2 @@
> >                                    aCallback, aClosure);
> >    }
> 
> These changes can be can reverted.

That caused build failures, I cleaned up our include story a bit and now we have to pass the origin as an arg.
Comment 4 Ed Morley [:emorley] 2012-06-20 02:18:45 PDT
https://hg.mozilla.org/mozilla-central/rev/30bfeaf1e175

Note You need to log in before you can comment on or make changes to this bug.