Closed Bug 786295 Opened 12 years ago Closed 12 years ago

Delete IndexedDB related to an app when uninstalled

Categories

(Core :: Storage: IndexedDB, defect, P1)

Product:
Core
Component:
Storage: IndexedDB
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox16 --- wontfix
firefox17 --- wontfix
firefox18 --- fixed

People

(Reporter: mounir, Assigned: bent.mozilla)

References

Details

(Whiteboard: [LOE:S][qa-])

Attachments

(1 file, 6 obsolete files)

Final patch
96.69 KB, patch
sicking
: review+
Details | Diff | Splinter Review 
      No description provided.
According to bent, it is not really easy. We should probably have someone who actually knows IndexedDB code to work on that?
Assignee: mounir → nobody
Status: ASSIGNED → NEW
Attached patch WIP Patch (obsolete) — — Splinter Review 
This patch is defining some methods. There is no rocket science. DatabaseInfo should be okay. IndexedDatabaseManager::ClearDatabasesForApp is just a copy of IndexedDatabaseManager::ClearDatabasesForURI with minor changes. That is the biggest pain because making this working isn't trivial according to bent.
I think this is privacy-related and AFAIK is a v1 blocker according to product people.
blocking-basecamp: ? → +
Whiteboard: [LOE:S]
If you're not the correct owner here, Mounir, please suggest a better person.
Assignee: nobody → mounir
Ben, can you take this?
Assignee: mounir → bent.mozilla
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P1]
Keywords: feature
> This patch is defining some methods. There is no rocket science.
> DatabaseInfo should be okay. IndexedDatabaseManager::ClearDatabasesForApp is
> just a copy of IndexedDatabaseManager::ClearDatabasesForURI with minor
> changes. That is the biggest pain because making this working isn't trivial
> according to bent.

Bent, can you tell me something more?
It's ok, I think I'm taking this as part of bug 792892.
Oh, but the thing that makes this more complicated is that we have to a) ensure that no current database activity is happening, and wait if it is, then b) we have to figure out how to find all the databases associated with an app (basically we have to iterate the whole indexedDB directory I think).
First we need to make IndexedDB aware of app ids. This tightens up child process security as well.

Next we need to do the actual deletion.
Attachment #656137 - Attachment is obsolete: true
Attachment #664362 - Flags: review?(khuey)
Attachment #664362 - Flags: review?(jonas)
Attachment #664362 - Attachment description: Make IndexedDB aware of app ids, v1 → Part 1: Make IndexedDB aware of app ids, v1
Comment on attachment 664362 [details] [diff] [review]
Part 1: Make IndexedDB aware of app ids, v1

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

::: dom/indexedDB/IDBFactory.cpp
@@ +137,3 @@
>  
>    nsCString origin(aASCIIOrigin);
>    if (origin.IsEmpty()) {

Does this actually happen?  If so, why?

@@ +137,4 @@
>  
>    nsCString origin(aASCIIOrigin);
>    if (origin.IsEmpty()) {
> +    rv = principal->GetAppId(&aAppId);

Using arguments as scratch space makes khuey sad.

@@ +142,5 @@
> +
> +    NS_ASSERTION(aAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
> +                 "Bad appId!");
> +
> +    rv = principal->GetIsInBrowserElement(&aIsBrowser);

Ditto.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +995,3 @@
>  
> +  principal.forget(aPrincipal);
> +  return NS_OK;

Given that we always return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, can we ditch the outparam bit and just return the principal directly?

::: dom/ipc/Makefile.in
@@ +102,5 @@
>  	-I$(topsrcdir)/dom/devicestorage \
>  	-I$(topsrcdir)/widget/xpwidgets \
>  	-I$(topsrcdir)/dom/bluetooth \
>  	-I$(topsrcdir)/dom/bluetooth/ipc \
> +	-I$(topsrcdir)/caps/include \

nsIPrincipal.h and nsIScriptSecurityManager.h are not exported?  I find that quite surprising.

::: dom/ipc/TabParent.cpp
@@ +900,5 @@
> +                 "This TabParent has a bad appId!");
> +
> +    if (appId != aAppId) {
> +      NS_WARNING("Received IndexedDB request from a different app!");
> +      return false;

Does returning false kill the calling process?  Seems like this is the kind of thing that should be fatal.
Attachment #664362 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Does this actually happen?  If so, why?

It always happens for same-process windows. Other-process windows already know their origin.

> Using arguments as scratch space makes khuey sad.

I don't feel strongly so I changed it (though my way was much less verbose).

> Given that we always return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, can we ditch
> the outparam bit and just return the principal directly?

Ok.

> nsIPrincipal.h and nsIScriptSecurityManager.h are not exported?  I find that
> quite surprising.

C'est la vie.

> Does returning false kill the calling process?  Seems like this is the kind
> of thing that should be fatal.

Yes, and it is.
(In reply to ben turner [:bent] from comment #11)
> > nsIPrincipal.h and nsIScriptSecurityManager.h are not exported?  I find that
> > quite surprising.
> 
> C'est la vie.

Perhaps I should have been more direct.  I meant that they are exported, and mucking with LOCAL_INCLUDES should be unnecessary, and you should stop doing that before checking this in.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> Perhaps I should have been more direct.  I meant that they are exported, and
> mucking with LOCAL_INCLUDES should be unnecessary, and you should stop doing
> that before checking this in.

Oh, I misunderstood. I need 'GetExtendedOrigin' in nsScriptSecurityManager.h.
Yuck.  We should move that to a {C++ } block in nsIScriptSecurityManager.idl.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #14)
> Yuck.  We should move that to a {C++ } block in nsIScriptSecurityManager.idl.

If you feel strongly please file a followup :)
Given that we have all the other data-clearing, we can treat this as a bug.
Keywords: feature
How is this coming along?  Its one of the main core security bugs left for basecamp.
Attached patch Patch, v2 (obsolete) — — Splinter Review 
This is about as far as I can go at the moment. Flagging three different folks for review:

bz, can you look over the changes to caps? I added the 'isNullPrincipal' function that we discussed.

jlebar, can you look over the nsFrameLoader changes? Those changes make the child process remember its appid.

khuey, you get everything else :)
Attachment #664362 - Attachment is obsolete: true
Attachment #670056 - Flags: review?(khuey)
Attachment #670056 - Flags: review?(justin.lebar+bug)
Attachment #670056 - Flags: review?(bzbarsky)
Comment on attachment 670056 [details] [diff] [review]
Patch, v2

Caps changes look fine to me.
Attachment #670056 - Flags: review?(bzbarsky) → review+
Comment on attachment 670056 [details] [diff] [review]
Patch, v2

I'm not a fan of NS_ASSERTION's for null pointers.  We gain very little from
them, as if the assertion fails, we'll quickly hit a null-pointer exception and
crash.

I'm also not a fan of NS_ASSERTION's in general.  We tend to ignore them,
particularly on B2G, where we usually don't have a console.   If it's
important, MOZ_ASSERT it.  Otherwise, why are you asserting it?

Anyway, I'm going to do two passes over this file; the first covers nits, and
the second covers functionality.

**** NITS ****

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>@@ -1974,38 +1974,66 @@ nsFrameLoader::TryRemoteBrowser()
>   }
>   if (NS_FAILED(window->GetChromeFlags(&chromeFlags))) {
>     return false;
>   }
> 
>   bool isBrowserElement = false;
>   nsCOMPtr<mozIApplication> app;
>   if (OwnerIsBrowserFrame()) {

It's particularly confusing here that nsFrameLoader has an "IsBrowserFrame"
method which returns "browsertab || app", and then we set "isBrowserElement" to
"browsertab" (or "browsertab && !app", if you prefer).

We don't distinguish between "browser frame" and "browser element" in this way;
everywhere else, those two terms are used interchangeably.

If you don't want to clean up the bigger mess, that's OK, but can you maybe
call the variable isBrowserNotApp or something?

On an unrelated note, we could /really/ use a term which clearly means "browser
or app".  Ideas would be greatly appreciated.

>-    isBrowserElement = true;
>+    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>+    if (!appsService) {
>+      NS_ERROR("Apps Service is not available!");
>+      return false;
>+    }

You could do NS_ENSURE_TRUE(appsService, false) to the same effect.

>     nsAutoString manifest;
>     GetOwnerAppManifestURL(manifest);
>-    if (!manifest.IsEmpty()) {
>-      nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>-      if (!appsService) {
>-        NS_ERROR("Apps Service is not available!");
>+
>+    nsCOMPtr<mozIDOMApplication> domApp;

Please add a comment explaining what this variable is (it's either the app
specified by the iframe's manifest, or the app which contains the iframe).

>+    if (manifest.IsEmpty()) {
>+      nsIPrincipal* ownerPrincipal = mOwnerContent->NodePrincipal();
>+      NS_ASSERTION(ownerPrincipal, "No principal?!");

Please remove the null-check assertion which immediately preceeds use of the
pointer.  Anyway, NodePrincipal() doesn't have "Get" in front of it because
it's promising not to return null.

>+      uint32_t appId;
>+      if (NS_FAILED(ownerPrincipal->GetAppId(&appId))) {
>+        NS_ERROR("This shouldn't fail!");
>         return false;
>       }

Again NS_ENSURE_SUCCESS(ownerPrincipal->GetAppId(&appId), false) is more
concise.  Even better would be to make the attribute on nsIPrincipal
[infallible], if it actually never fails.

>-      nsCOMPtr<mozIDOMApplication> domApp;
>+      NS_ASSERTION(appId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
>+                   "Must have a known app id here!");

>+      if (appId != nsIScriptSecurityManager::NO_APP_ID) {
>+        if (NS_FAILED(appsService->GetAppByLocalId(appId,
>+                                                   getter_AddRefs(domApp)))) {
>+          NS_ERROR("appsService failed to find the right app!");
>+          return false;
>+        }

NS_ENSURE_SUCCESS?
>+    if (domApp) {
>       app = do_QueryInterface(domApp);
>-      if (app) {
>-        isBrowserElement = false;
>-      }
>+      NS_ASSERTION(app, "domApp doesn't QI to mozIApplication!");
>     }
>   }

**** FUNCTIONALITY ****

Don't you need to change MaybeCreateDocShell() for the in-process case?  Right
now, it looks like we always call SetIsBrowserElement() for (browser-tab ||
app).

Maybe this code would be clearer (and the bug above impossible) if you merged
SetIsBrowserElement() and SetAppId() in nsIDocShell.  You have to set both at
the same time, and you cannot call that new method twice.  Here's where having
a good name would help a lot.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>@@ -1974,38 +1974,66 @@ nsFrameLoader::TryRemoteBrowser()
>   }
>   if (NS_FAILED(window->GetChromeFlags(&chromeFlags))) {
>     return false;
>   }
> 
>   bool isBrowserElement = false;
>   nsCOMPtr<mozIApplication> app;
>   if (OwnerIsBrowserFrame()) {
>-    isBrowserElement = true;
>+    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>+    if (!appsService) {
>+      NS_ERROR("Apps Service is not available!");
>+      return false;
>+    }
> 
>     nsAutoString manifest;
>     GetOwnerAppManifestURL(manifest);
>-    if (!manifest.IsEmpty()) {
>-      nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>-      if (!appsService) {
>-        NS_ERROR("Apps Service is not available!");
>+
>+    nsCOMPtr<mozIDOMApplication> domApp;
>+    if (manifest.IsEmpty()) {
>+      nsIPrincipal* ownerPrincipal = mOwnerContent->NodePrincipal();
>+      NS_ASSERTION(ownerPrincipal, "No principal?!");
>+
>+      uint32_t appId;
>+      if (NS_FAILED(ownerPrincipal->GetAppId(&appId))) {
>+        NS_ERROR("This shouldn't fail!");
>         return false;
>       }
> 
>-      nsCOMPtr<mozIDOMApplication> domApp;
>+      NS_ASSERTION(appId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
>+                   "Must have a known app id here!");

This assertion is true only if all browser frames must be contained inside app
frames, right?  That's certainly not true (for example in most of the
browser-frame mochitests).

(This is a great argument for using MOZ_ASSERT, btw, because mochitest doesn't
die on NS_ASSERT, so we never would have noticed that this assertion was
wrong.)

Anyway I'm concerned about the following possibility.  Suppose we have an app
iframe as follows:

>  <iframe mozbrowser mozapp="http://app.com/manifest" src="http://somewhereelse.com">

Now somewhereelse.com creates <iframe mozbrowser>.  (Suppose that URL has
permission; it could.)

Now the app-id of the somewhereelse.com principal is app.com's app-id, but the
principal's app-status is NOT_INSTALLED.  That's how we handle app frames which
are outside their "natural origin".

So the question is, should the browser frame inherit app.com's app-id?  That
would be wrong if we gave the browser frame extra privileges by virtue of the
fact that it's part of app.com's app, since app.com did not sanction the
creation of the browser frame.

But if it shouldn't have app.com's app-id, what app-id /should/ the browser
frame have?  It shouldn't be 0, because that would break storage.

It's because questions like this are so hard that I'm increasingly convinced
we're working at the wrong abstraction here.

Maybe we can punt on this for now and require that if we have an app-id, then
we must also have app-status >= INSTALLED.  That is, off-origin app content
can't create browsers.

>+      if (appId != nsIScriptSecurityManager::NO_APP_ID) {
>+        if (NS_FAILED(appsService->GetAppByLocalId(appId,
>+                                                   getter_AddRefs(domApp)))) {
>+          NS_ERROR("appsService failed to find the right app!");
>+          return false;
>+        }

I think you can MOZ_ASSERT this.

>+        NS_ASSERTION(domApp, "Null app!");

Isn't it an XPCOM rule that you return success iff you set your out param?  So
you shouldn't need to check both.

>+        // This is always marked as a browser element.
>+        isBrowserElement = true;

Can you move this right under the manifest.IsEmpty() check?  Also, rather than
a functional comment, "This is always marked as a browser element," (I could
tell), could you explain /why/ this is a browser element?  (If you rename it to
isBrowserNotApp, maybe it will be obvious and won't need a comment.)


Sorry for an overly-long review.  Mind if I take another look?
Attachment #670056 - Flags: review?(justin.lebar+bug) → review-
Comment on attachment 670056 [details] [diff] [review]
Patch, v2

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

I didn't look at all of it, but there's enough changes that I want to see it again.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +156,5 @@
> +
> +// Use one of the friendly overloads below.
> +void
> +GetOriginPatternString(uint32_t aAppId, MozBrowserPatternFlag aBrowserFlag,
> +                       const nsACString& aOrigin, nsAutoCString& _retval)

This should just be an nsCString& or an nsACString&, not an nsAutoCString&.

@@ +163,5 @@
> +               "Bad appId!");
> +  NS_ASSERTION(aBrowserFlag == MozBrowser ||
> +               aBrowserFlag == NotMozBrowser ||
> +               aBrowserFlag == IgnoreMozBrowser,
> +               "Bad args!");

Asserting that your enum type is one of the enum values is silly.

@@ +173,5 @@
> +
> +    _retval.AppendInt(aAppId);
> +    _retval.Append('+');
> +
> +    if (aBrowserFlag != IgnoreMozBrowser) {

either do if else if else or a switch, this nested if is silly.

@@ +287,4 @@
>  PLDHashOperator
> +InvalidateAndRemoveFileManagers(
> +                           const nsACString& aKey,
> +                           nsAutoPtr<nsTArray<nsRefPtr<FileManager> > >& aValue,

Why did you need to change this to an nsAutoPtr?  At least make it a const nsAutoPtr so you can't accidentally delete the array of file managers.

@@ +320,5 @@
> +#ifdef XP_WIN
> +  NS_ASSERTION(!strcmp(kReplaceChars,
> +                       FILE_ILLEGAL_CHARACTERS FILE_PATH_SEPARATOR),
> +               "Illegal file characters have changed!");
> +#endif

Hrm, don't we want this to be some sort of static assertion?

@@ +332,5 @@
> +                      bool aInMozBrowser,
> +                      nsACString& aOrigin)
> +{
> +  NS_ASSERTION(aURI, "Null uri!");
> +  

Extra whitespace.

@@ -475,3 @@
>    NS_ASSERTION(aRunnable, "Null pointer!");
>  
> -  nsAutoPtr<SynchronizedOp> op(new SynchronizedOp(aOrigin, aId));

What's the point of moving this later?  The only time this allocation is not necessary is when NS_DispatchToCurrentThread fails, which should basically never happen.

@@ +730,5 @@
>    bool delayed = false;
>    for (uint32_t index = mSynchronizedOps.Length(); index > 0; index--) {
>      nsAutoPtr<SynchronizedOp>& existingOp = mSynchronizedOps[index - 1];
> +    if (SynchronizedOp::MustWaitFor(*existingOp, aOriginOrPattern, aIsOrigin,
> +                                    aId)) {

And then not having a SynchronizedOp ends up causing this nastiness.

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +57,5 @@
>  
>    // Waits for databases to be cleared and for version change transactions to
>    // complete before dispatching the given runnable.
> +  nsresult WaitForOpenAllowed(const nsACString& aOriginOrPattern,
> +                              bool aIsOrigin,

Nope.  Opaque boolean parameters are no fun.  Please use an enum with eOrigin/ePattern or something.

@@ +210,5 @@
> +
> +  static bool
> +  OriginMatchesApp(const nsACString& aOrigin,
> +                   uint32_t aAppId,
> +                   bool aInMozBrowser);

Yuck more booleans.  There should be a global enum somewhere for is/is not in mozbrowser.

@@ +236,5 @@
>  
>    // Called when a database has been closed.
>    void OnDatabaseClosed(IDBDatabase* aDatabase);
>  
> +  nsresult ClearDatabasesForApp(uint32_t aAppId, bool aBrowserOnly);

Enum!

@@ +332,5 @@
>      NS_DECL_ISUPPORTS
>      NS_DECL_NSIRUNNABLE
>  
> +    AsyncUsageRunnable(uint32_t aAppId,
> +                       bool aInMozBrowserOnly,

enum

@@ +386,5 @@
>    // clearing dbs for an origin, etc).
>    struct SynchronizedOp
>    {
> +    SynchronizedOp(const nsACString& aOriginOrPattern,
> +                   bool aIsOrigin,

enum!

@@ +394,5 @@
>      // Test whether the second SynchronizedOp needs to get behind this one.
> +    static bool MustWaitFor(const SynchronizedOp& aOp,
> +                            const nsACString& aOriginOrPattern,
> +                            bool aIsOrigin,
> +                            nsIAtom* aId);

The changes to MustWaitFor are unnecessary.  Please revert them.

@@ +405,5 @@
>      nsRefPtr<AsyncConnectionHelper> mHelper;
>      nsCOMPtr<nsIRunnable> mRunnable;
>      nsTArray<nsCOMPtr<nsIRunnable> > mDelayedRunnables;
>      nsTArray<IDBDatabase*> mDatabases;
> +    bool mIsOrigin;

enum!
Attachment #670056 - Flags: review?(khuey)
Attachment #670056 - Flags: review?(justin.lebar+bug)
Attachment #670056 - Flags: review-
(In reply to Justin Lebar [:jlebar] from comment #20)
> I'm not a fan of NS_ASSERTION's for null pointers.

Ok.

> I'm also not a fan of NS_ASSERTION's in general.

Well, for one thing, we didn't get stack traces on tinderbox for MOZ_ASSERT stuff a little while ago... We might not still (I'm having trouble finding the bug). Also, I really think it's more confusing to have a file with mixed NS_ASSERTION and MOZ_ASSERT. However, this isn't my code and I won't argue it here.

> If you don't want to clean up the bigger mess, that's OK, but can you maybe
> call the variable isBrowserNotApp or something?

I really don't want to tackle the underlying architectural issues here. We can do it in a followup.

> You could do NS_ENSURE_TRUE(appsService, false) to the same effect.

Well, the NS_ENSURE_ macros issue a warning and in this spot and the others below I thought these things should be asserted. But I'm not going to argue.

> Don't you need to change MaybeCreateDocShell() for the in-process case? 
> Right
> now, it looks like we always call SetIsBrowserElement() for (browser-tab ||
> app).

Can we worry about this in a followup? I'm really only concerned with B2G here, and I don't know if we have use cases for in-process browser/app stuff yet. Do we?

> Maybe this code would be clearer (and the bug above impossible) if you merged
> SetIsBrowserElement() and SetAppId() in nsIDocShell.  You have to set both at
> the same time, and you cannot call that new method twice.  Here's where
> having
> a good name would help a lot.

This is part of the plan for the followup, basically. I don't want to change it here though.

> This assertion is true only if all browser frames must be contained inside
> app
> frames, right?  That's certainly not true (for example in most of the
> browser-frame mochitests).

Shouldn't their appId be NO_APP_ID? I was under the impression that UNKNOWN_APP_ID shouldn't ever be returned here.

> Anyway I'm concerned about the following possibility.  Suppose we have an app
> iframe as follows:
> ...
> Maybe we can punt on this for now and require that if we have an app-id, then
> we must also have app-status >= INSTALLED.

Anything we can do later we should do later imo.

> >+        NS_ASSERTION(domApp, "Null app!");
> 
> Isn't it an XPCOM rule that you return success iff you set your out param? 
> So
> you shouldn't need to check both.

It has to be set, yes, but there's no rule saying what it has to be set to. Setting to null is perfectly fine in the XPCOM rulebook and makes sense in a lot of APIs. Here I have no idea, I'm just defensive on these things. I'll remove it if you want me to.

> Sorry for an overly-long review.  Mind if I take another look?

No worries, new patch up in a bit.
> Well, the NS_ENSURE_ macros issue a warning and in this spot and the others below I thought these 
> things should be asserted.

I don't understand the difference.  NS_ENSURE_ is going to output a non-fatal debug-only warning to the command-line, and the original NS_ERROR is going to do the same thing, right?  The only difference I see is how many lines of code each approach takes (and perhaps whether you get a stack?), but maybe I'm missing something.

> Can we worry about this in a followup? I'm really only concerned with B2G here, and I don't know if 
> we have use cases for in-process browser/app stuff yet. Do we?

B2G uses in-process browser frames extensively.  You definitely should be testing that, if you're not.

> Shouldn't their appId be NO_APP_ID? I was under the impression that UNKNOWN_APP_ID shouldn't ever 
> be returned here.

You're right; I wasn't even aware of this distinction.

> It has to be set, yes, but there's no rule saying what it has to be set to. Setting to null is 
> perfectly fine in the XPCOM rulebook and makes sense in a lot of APIs. Here I have no idea, I'm 
> just defensive on these things. I'll remove it if you want me to.

Maybe you could just check for truthiness of the outparam?  Is that safe according to the rules?  Being defensive needs to be balanced with writing readable code, is all.
NS_ASSERTION has not given (useful) stacks on tinderbox for months.  See Bug 755781.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #21)

> This should just be an nsCString& or an nsACString&, not an nsAutoCString&.

What's your reason for suggesting that? Just convention? In this case the function's whole purpose is to concatenate a bunch of stuff and I don't want a caller to pass in a string that will allocate every time I call Append.

> Asserting that your enum type is one of the enum values is silly.

Yeah, I think it was muscle memory from dealing with XPIDL "enums".

> either do if else if else or a switch, this nested if is silly.

You saw that final Append('+') that has to happen for both cases right? I don't think this construction is silly at all, please explain your objection better.

> Why did you need to change this to an nsAutoPtr?  At least make it a const
> nsAutoPtr so you can't accidentally delete the array of file managers.

I don't get to pick the function signatures of our non-const hashtable enum functions. This won't compile any other way.

> Hrm, don't we want this to be some sort of static assertion?

How would you do that?

> What's the point of moving this later?  The only time this allocation is not
> necessary is when NS_DispatchToCurrentThread fails, which should basically
> never happen.

It's less about how it can fail and more about having the MustWaitFor be a static function. It's really confusing looking at the MustWaitFor function trying to figure out if 'aRHS' is the one that needs to wait or if 'this' is the one that needs to wait. Making it a static function is easier imo.

> And then not having a SynchronizedOp ends up causing this nastiness.

I don't think calling a static function is really all that nasty, but even if you do I think the clarity of the static function outweighs it.

> Nope.  Opaque boolean parameters are no fun.  Please use an enum with
> eOrigin/ePattern or something.

Meh. I don't care too strongly.

So this r- is basically a bunch of nits, please let's get to the meat of the patch with the next version? Time is not on our side here.
(In reply to Justin Lebar [:jlebar] from comment #23)
> The only difference I see is how many
> lines of code each approach takes (and perhaps whether you get a stack?),
> but maybe I'm missing something.

On Linux and Mac, yes, they just print a stack and continue (maybe you get a bell on mac, I can't remember). On Windows the behavior is the same on tinderbox (due to an env variable) but when actually running a debug build assertion failures are an ugly modal dialog that allow you to step in in a debugger and are much harder to ignore.

> B2G uses in-process browser frames extensively.  You definitely should be
> testing that, if you're not.

Sadface. I'll find you on irc to chat about that, this is not something I've run into yet.
I don't feel like a stack is really important in this case, myself.  We have to weigh the verbosity of the code against the probability of the assertion ever failing, which we think is basically 0.
(In reply to ben turner [:bent] from comment #25)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #21)
> 
> > This should just be an nsCString& or an nsACString&, not an nsAutoCString&.
> 
> What's your reason for suggesting that? Just convention? In this case the
> function's whole purpose is to concatenate a bunch of stuff and I don't want
> a caller to pass in a string that will allocate every time I call Append.

The 'auto'ness of the string is tracked in the flags, not in the type, so using an nsCString or nsACString reference does not affect that.  Confirm with dbaron or NeilAway.

> > either do if else if else or a switch, this nested if is silly.
> 
> You saw that final Append('+') that has to happen for both cases right? I
> don't think this construction is silly at all, please explain your objection
> better.

Ah.  Guess there's no better way to do this then.

> > Why did you need to change this to an nsAutoPtr?  At least make it a const
> > nsAutoPtr so you can't accidentally delete the array of file managers.
> 
> I don't get to pick the function signatures of our non-const hashtable enum
> functions. This won't compile any other way.

I'm *really* surprised that you can't make it const.  Passing a non-const type Foo to a function that takes a const Foo should just work ...

> > Hrm, don't we want this to be some sort of static assertion?
> 
> How would you do that?

I'm not sure that you can without C++0x :-(.  At least make this a hard abort so we'll be sure to notice.

> > What's the point of moving this later?  The only time this allocation is not
> > necessary is when NS_DispatchToCurrentThread fails, which should basically
> > never happen.
> 
> It's less about how it can fail and more about having the MustWaitFor be a
> static function. It's really confusing looking at the MustWaitFor function
> trying to figure out if 'aRHS' is the one that needs to wait or if 'this' is
> the one that needs to wait. Making it a static function is easier imo.

The current semantics are quite clear.  foo->MustWaitFor(bar) is asking must foo wait for bar.  It's even clearer in context because the argument is named "existingOp", and the 'this' object was just allocated.

> > And then not having a SynchronizedOp ends up causing this nastiness.
> 
> I don't think calling a static function is really all that nasty, but even
> if you do I think the clarity of the static function outweighs it.

Well you're wrong ;-)
Comment on attachment 670056 [details] [diff] [review]
Patch, v2

I'm presuming that Kyle midaired my r-.
Attachment #670056 - Flags: review?(justin.lebar+bug) → review-
> The 'auto'ness of the string is tracked in the flags, not in the type, so using an nsCString or 
> nsACString reference does not affect that.  Confirm with dbaron or NeilAway.

I'm pretty sure that:

 - Using nsACString& doesn't affect the auto-ness of the string.
 - You can't cast nsAutoCString& to nsCString&.

But I didn't look at any code.
Priority: -- → P1
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S]
Attached patch Patch, v2.1 (obsolete) — — Splinter Review 
Updated to address review comments.
Attachment #670056 - Attachment is obsolete: true
Attachment #671630 - Flags: review?(khuey)
Attachment #671630 - Flags: review?(justin.lebar+bug)
Comment on attachment 671630 [details] [diff] [review]
Patch, v2.1

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

r- for the bit in AllowNextSynchronizedOp.  I'll change it to r+ if you prove that I'm wrong.

::: dom/indexedDB/IDBFactory.cpp
@@ +540,5 @@
>  
>      IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
>      NS_ASSERTION(mgr, "This should never be null!");
>  
>      rv = 

Since you're here, remove the extra whitespace on this line?

::: dom/indexedDB/IndexedDatabase.h
@@ +208,5 @@
> +  { }
> +
> +  // Don't want this.
> +  bool
> +  operator==(const OriginOrPatternString& aOther);

Mark this with MOZ_DELETE.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +315,5 @@
> +
> +#ifdef XP_WIN
> +  NS_ASSERTION(!strcmp(kReplaceChars,
> +                       FILE_ILLEGAL_CHARACTERS FILE_PATH_SEPARATOR),
> +               "Illegal file characters have changed!");

MOZ_ASSERT

@@ +757,5 @@
>  
>    uint32_t count = mSynchronizedOps.Length();
>    for (uint32_t index = 0; index < count; index++) {
>      nsAutoPtr<SynchronizedOp>& op = mSynchronizedOps[index];
> +    if (op->mOriginOrPattern == aOrigin) {

Doesn't this need to check PatternMatchesOrigin or something?

@@ +798,5 @@
>    NS_ASSERTION(!op->mHelper, "SynchronizedOp already has a helper?!?");
>    NS_ASSERTION(!op->mRunnable, "SynchronizedOp already has a runnable?!?");
>  
> +  DatabasePatternMatchArray matches;
> +  matches.Find(mLiveDatabases, aPattern);

/me mumbles about hash tables being the wrong data structure.

@@ +1416,5 @@
> +        (!currentOp->mId || currentOp->mId == aId)) {
> +      return currentOp;
> +    }
> +  }
> +  return nullptr;

Super nit: \n before return.

@@ +1434,5 @@
> +  GetOriginPatternStringMaybeIgnoreBrowser(aAppId, aBrowserOnly, pattern);
> +
> +  // If there is a pending or running clear operation for this app, return
> +  // immediately.
> +  if (IsClearOriginPending(pattern)) {

We probably should rename IsClearOriginPending at some point.  Maybe IsClearOriginInProgress?

@@ +1482,5 @@
> +  NS_ENSURE_TRUE(IsMainProcess(), NS_ERROR_NOT_AVAILABLE);
> +
> +  if (!aOptionalArgCount) {
> +    aAppId = nsIScriptSecurityManager::NO_APP_ID;
> +  }

Shouldn't you provide a default for aInMozBrowserOnly?

@@ -1201,5 @@
> -  // Non-standard URIs can't create databases anyway so fire the callback
> -  // immediately.
> -  if (origin.EqualsLiteral("null")) {
> -    return runnable->TakeShortcut();
> -  }

Why did you get rid of this?

@@ +1523,5 @@
> +  // This only works from the main process.
> +  NS_ENSURE_TRUE(IsMainProcess(), NS_ERROR_NOT_AVAILABLE);
> +
> +  if (!aOptionalArgCount) {
> +    aAppId = nsIScriptSecurityManager::NO_APP_ID;

Same thing about aInMozBrowserOnly.

@@ +1560,5 @@
> +  // This only works from the main process.
> +  NS_ENSURE_TRUE(IsMainProcess(), NS_ERROR_NOT_AVAILABLE);
> +
> +  if (!aOptionalArgCount) {
> +    aAppId = nsIScriptSecurityManager::NO_APP_ID;

And here.
Attachment #671630 - Flags: review?(khuey) → review-
Comment on attachment 671630 [details] [diff] [review]
Patch, v2.1

This is a big improvement over the existing code.  Thanks.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>@@ -325,16 +326,23 @@ private:
>   /**
>+   * Get the application associated with this frame. The extra outparam tells
>+   * whether the frame represents a real app or simply the app that owns this
>+   * frame.

This isn't parallel as written.  (Try re-ordering around the "or" and you get
"tells whether simply the app that owns this frame.")  Maybe instead:

  The outparam is true if this frame is a browser embedded inside the returned
  app (or if this frame is a browser not embedded inside any app).  The
  outparam is false if this frame is itself an app frame or if this frame is
  neither an app nor a browser frame.

>+  already_AddRefed<mozIApplication> GetApp(bool* aIsBrowserNotApp);

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -1411,16 +1411,66 @@ nsFrameLoader::GetOwnerAppManifestURL(ns
> {
>   aOut.Truncate();
>   nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(mOwnerContent);
>   if (browserFrame) {
>     browserFrame->GetAppManifestURL(aOut);
>   }
> }
> 
>+already_AddRefed<mozIApplication>
>+nsFrameLoader::GetApp(bool* aIsBrowserNotApp)
>+{
>+  *aIsBrowserNotApp = false;
>+
>+  nsCOMPtr<mozIApplication> app;
>+  if (OwnerIsBrowserFrame()) {

We prefer early returns to large branches, so make this 

  if (!OwnerIsBrowserFrame()) { return nullptr; }.

>+    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>+    NS_ENSURE_TRUE(appsService, nullptr);
>+
>+    nsAutoString manifest;
>+    GetOwnerAppManifestURL(manifest);
>+
>+    // This is either the app specified by the iframe's manifest or the app that
>+    // contains the iframe.
>+    nsCOMPtr<mozIDOMApplication> domApp;
>+    if (manifest.IsEmpty()) {
>+      // No manifest means that we're really a browser, not an app.

Maybe add "Now see if we're contained inside an app."

>+      *aIsBrowserNotApp = true;
>+
>+      uint32_t appId;
>+      NS_ENSURE_SUCCESS(mOwnerContent->NodePrincipal()->GetAppId(&appId),
>+                        nullptr);
>+
>+      NS_ASSERTION(appId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
>+                   "Must have a known app id here!");
>+
>+      if (appId != nsIScriptSecurityManager::NO_APP_ID) {
>+        appsService->GetAppByLocalId(appId, getter_AddRefs(domApp));
>+        MOZ_ASSERT(domApp);
>+      }
>+    } else {
>+      appsService->GetAppByManifestURL(manifest, getter_AddRefs(domApp));
>+
>+      // If the frame is actually an app, we should not mark it as a
>+      // browser.  This is to identify the data store: since <app>s
>+      // and <browser>s-within-<app>s have different stores, we want
>+      // to ensure the <app> uses its store, not the one for its
>+      // <browser>s.
>+      if (!domApp) {
>+        *aIsBrowserNotApp = true;
>+      }

I think this comment is now confusing (and not entirely applicable).  I think
it's clear without any comment why *aIsBrowserNotApp is true here, but you
could write a new comment if you think it would help.

>+    app = do_QueryInterface(domApp);

Hm.  It seems like you should MOZ_ASSERT_IF(domApp, app), because this method
will send a very wrong signal if this QI fails.  Sorry if I said something
different earlier.

>@@ -1601,17 +1644,17 @@ nsFrameLoader::MaybeCreateDocShell()
>   EnsureMessageManager();
> 
>-  if (OwnerIsBrowserFrame()) {
>+  if (app && isBrowserNotApp) {
>     mDocShell->SetIsBrowserElement();
> 
>     nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>     if (os) {
>       os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
>                           "in-process-browser-frame-shown", NULL);
>     }

Why the |app &&| condition?  Shouldn't it just be |isBrowserNotApp|?  I'd
expect this would fail all the browser-frame tests as written.
(dom/browser-element/mochitest)

>+  if ((mRemoteBrowser = ContentParent::CreateBrowser(app, isBrowserNotApp))) {

So...for a browser embedded inside an app, does TabChild::SetAppBrowserConfig()
get a non-null app?  Without reading your whole patch, it seems like it does.
That would be bad, because we'll do docshell->SetAppId(appId), which you
explicitly avoid in the in-process case:

>@@ -1482,32 +1532,25 @@ nsFrameLoader::MaybeCreateDocShell()
>+  bool isBrowserNotApp;
>+  nsCOMPtr<mozIApplication> app = GetApp(&isBrowserNotApp);
>+  // Set the appId only if this is a real app frame.
>+  if (app && !isBrowserNotApp) {
>+    uint32_t appId;
>+    NS_ENSURE_SUCCESS(app->GetLocalId(&appId), NS_ERROR_FAILURE);
>+    mDocShell->SetAppId(appId);
>   }

clearing the r? until we get this resolved.

It would be easier for me to follow if you at least changed everywhere that
accepts this isBrowserNotApp variable to be called isBrowserNotApp.  As is,
they're still called isBrowserElement, but you've changed the meaning of that
variable in...some places.  Not everywhere.  Maybe.  Unclear...
Attachment #671630 - Flags: review?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #33)
Ok, I talked with sicking and we both think this needs to be split out into its own bug so that we can get this in. I've moved the frameloader changes to bug 802366.
Attached patch Patch, v2.2 (obsolete) — — Splinter Review 
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> r- for the bit in AllowNextSynchronizedOp.  I'll change it to r+ if you
> prove that I'm wrong.

Yeah, you're wrong :) I've changed the argument name and type to make it more clear though.

> MOZ_ASSERT

Please file a separate bug to switch all NS_ASSERTION in IndexedDB to MOZ_ASSERT. They're all supposed to be fatal.

> Shouldn't you provide a default for aInMozBrowserOnly?

No, false is the default we want.

> Why did you get rid of this?

This is dead code now.
Attachment #671630 - Attachment is obsolete: true
Attachment #672416 - Flags: review?(khuey)
Attached patch Patch, v2.3 (obsolete) — — Splinter Review 
Ok, this is the last one. I added a crazy test and found some fun stuff along the way. Specifically:

1. The other components hooked into the clear-private-data system expect to never see appId 0, but that should be expected and fine. I had to change them.

2. The OOP IndexedDB stuff never expects an Abort event to fire on a transaction without the errorCode being set, but that's also expected and fine. I updated the protocol to make that clear.

3. I had never hooked up Invalidate for OOP IndexedDB and that was causing stale metadata to stick around in the child process. I've fixed that here too.
Attachment #672416 - Attachment is obsolete: true
Attachment #673400 - Flags: review?(khuey)
Comment on attachment 673400 [details] [diff] [review]
Patch, v2.3

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

I did not review the test.

::: dom/indexedDB/IDBDatabase.h
@@ +107,5 @@
> +  // called on any thread.
> +  bool IsInvalidated() const
> +  {
> +    return mInvalidated;
> +  }

Do you really need to move this to the header?
Attachment #673400 - Flags: review?(khuey) → review+
Attached patch Final patch — — Splinter Review 
Small changes to SpecialPowers due to bug 785632. Grr.
Attachment #673400 - Attachment is obsolete: true
Attachment #674042 - Flags: review?(jonas)
Comment on attachment 674042 [details] [diff] [review]
Final patch

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

r=me on the last round of test fixes.
Attachment #674042 - Flags: review?(jonas) → review+
https://hg.mozilla.org/mozilla-central/rev/3eaf018fe2a0
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This bug is marked blocking-basecamp+, but the patch does not apply cleanly to Aurora. Please post a branch specific patch for checkin (or just push it yourself if you'd rather).
Whiteboard: [LOE:S] → [LOE:S][qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: