Closed Bug 961049 Opened 6 years ago Closed 4 years ago

Update quota manager to use PBackground

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(16 files)

338.54 KB, patch
Details | Diff | Splinter Review
9.15 KB, patch
luke
: review+
Details | Diff | Splinter Review
34.74 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
5.15 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.50 KB, patch
baku
: review+
Details | Diff | Splinter Review
7.12 KB, patch
baku
: review+
Details | Diff | Splinter Review
166.48 KB, patch
baku
: review+
Details | Diff | Splinter Review
69.47 KB, patch
baku
: review+
Details | Diff | Splinter Review
39.05 KB, patch
baku
: review+
Details | Diff | Splinter Review
9.42 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.58 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.71 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.12 KB, patch
Details | Diff | Splinter Review
17.05 KB, patch
Details | Diff | Splinter Review
5.14 KB, patch
Details | Diff | Splinter Review
2.15 KB, patch
Details | Diff | Splinter Review
No description provided.
Depends on: PBackground
Blocks: 961057
Blocks: 847913
Depends on: 942542
No longer blocks: AsyncIDB
No longer blocks: 771288
Depends on: 1173756
Attached patch patchSplinter Review
Kyle, Andrea, let me know which one of you is more comfortable with reviewing this patch. Thanks.
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Flags: needinfo?(khuey)
Flags: needinfo?(amarchesini)
Comment on attachment 8675278 [details] [diff] [review]
patch

Luke, please review dom/asmjscache changes.
Ben, please review dom/cache changes.
Attachment #8675278 - Flags: review?(luke)
Attachment #8675278 - Flags: review?(bkelly)
can you split the patch in smaller patches? It seems to me that there are a lot of independent code. Small patches are easy to follow (at least for me). Thanks!
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #3)
> can you split the patch in smaller patches? It seems to me that there are a
> lot of independent code. Small patches are easy to follow (at least for me).
> Thanks!

Ok, so are you willing to review all the changes (split into smaller patches), except dom/asmjscache and dom/cache ?
Comment on attachment 8675278 [details] [diff] [review]
patch

Since Jan is splitting patches, dropping the review flag for now.
Attachment #8675278 - Flags: review?(bkelly)
Attachment #8675278 - Flags: review?(luke)
Comment on attachment 8675810 [details] [diff] [review]
Part 6: Quota Manager on PBackground cache changes

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

Nice!  I don't see any major issues, just some extra cleanup we can do.  r=me with comments.

::: dom/cache/Context.cpp
@@ +378,5 @@
> +      nsCOMPtr<nsIPrincipal> principal = managerId->Principal();
> +      nsresult rv = QuotaManager::GetInfoFromPrincipal(principal,
> +                                                       &mQuotaInfo.mGroup,
> +                                                       &mQuotaInfo.mOrigin,
> +                                                       &mQuotaInfo.mIsApp);

Please file a follow-up bug to remove this GetInfoFromPrincipal().  Instead, we should just save all this information when we create the ManagerId:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/ManagerId.cpp#27

This will let us avoid a main thread dispatch.

Please set the follow-up bug to block bug 1110136.

::: dom/cache/Manager.cpp
@@ +248,2 @@
>  
> +    // The factory was destroyed between when abort started on main thread and

Is this comment still accurate?  It seems this can't happen any more because its all on the background thread.

@@ +314,1 @@
>      StaticMutexAutoLock lock(sMutex);

Do we still need sMutex?  It seems everything is running on a single thread now.
Attachment #8675810 - Flags: review?(bkelly) → review+
I'll let baku do the first pass, and I can review if more is needed.
Flags: needinfo?(khuey)
Attachment #8675808 - Flags: review?(luke) → review+
Attachment #8676866 - Flags: review?(amarchesini)
Attachment #8676868 - Flags: review?(amarchesini)
Attachment #8676866 - Flags: review?(amarchesini) → review+
Attachment #8676875 - Flags: review?(amarchesini)
Attachment #8675808 - Attachment description: dom/asmjscache changes → Part 5: QuotaManager on PBackground asmjscache changes
Attachment #8675810 - Attachment description: dom/cache changes → Part 6: Quota Manager on PBackground cache changes
Attachment #8676880 - Flags: review?(amarchesini)
Comment on attachment 8676868 [details] [diff] [review]
Part 2: Remove Utilities.h

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

::: dom/quota/UsageInfo.h
@@ +81,5 @@
> +  static void
> +  IncrementUsage(uint64_t* aUsage, uint64_t aDelta)
> +  {
> +    // Watch for overflow!
> +    if ((UINT64_MAX - *aUsage) < aDelta) {

This will not work.
do:

MOZ_ASSERT(aUsage);
CheckUint64 value = *aUsage;
value += aData;
if (value.isValid()) {
  *aUsage = value.value();
} else {
  *aUsage = UINT64_MAX;
}
Attachment #8676868 - Flags: review?(amarchesini) → review+
Attachment #8676871 - Flags: review?(amarchesini) → review+
Latest try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c521dbd16743

Just some usual intermittent failures and permaoranges.
Comment on attachment 8676875 [details] [diff] [review]
Part 4: QuotaManager on PBackground core changes

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

Let me know what you think about this approach of managing the state machine.
The rest looks good. As usual :)

::: dom/quota/ActorsChild.cpp
@@ +38,5 @@
> +
> +#ifdef DEBUG
> +
> +void
> +QuotaChild::AssertIsOnOwningThread() const

I have seen this pattern quite often in your code.
Would be nice to have a base class for this.

@@ +57,5 @@
> +
> +  if (mService) {
> +    mService->ClearBackgroundActor();
> +#ifdef DEBUG
> +    mService = nullptr;

any reason to do it only in debug builds?

@@ +154,5 @@
> +  AssertIsOnOwningThread();
> +
> +  if (mRequest) {
> +    mRequest->ClearBackgroundActor();
> +#ifdef DEBUG

ditto.

@@ +204,5 @@
> +
> +#ifdef DEBUG
> +
> +void
> +QuotaRequestChild::AssertIsOnOwningThread() const

yep, definitely would be nice to have a base class :)

@@ +223,5 @@
> +  mRequest->SetError(aResponse);
> +}
> +
> +void
> +QuotaRequestChild::HandleResponse(const RequestResponse& aResponse)

don't pass this argument if you don't want to use it.
Are you planning to use this in other patches? If not, maybe a better name can be chosen.

@@ +252,5 @@
> +    case RequestResponse::TClearOriginResponse:
> +    case RequestResponse::TClearAppResponse:
> +    case RequestResponse::TClearAllResponse:
> +    case RequestResponse::TResetAllResponse:
> +      HandleResponse(aResponse);

HandleResponse();

::: dom/quota/ActorsChild.h
@@ +38,5 @@
> +#endif
> +
> +public:
> +  void
> +  AssertIsOnOwningThread() const

Yep, a QuotaDebugging class ?

::: dom/quota/ActorsParent.cpp
@@ +347,5 @@
> +
> +  void
> +  AddCallback(nsIRunnable* aCallback)
> +  {
> +    AssertIsOnOwningThread();

MOZ_ASSERT(aCallback);

@@ +374,5 @@
> +
> +class QuotaManager::ShutdownRunnable final
> +  : public nsRunnable
> +{
> +  bool& mDone;

// touched only on the main-thread.

@@ +1033,5 @@
> +  virtual bool
> +  Init(Quota* aQuota);
> +
> +protected:
> +  QuotaRequestBase(bool aExclusive)

explicit

@@ +1081,3 @@
>  };
>  
>  class OriginClearOp

final ?

@@ +2230,5 @@
> +  nsTArray<nsCOMPtr<nsIRunnable>> callbacks;
> +  mCallbacks.SwapElements(callbacks);
> +
> +  for (nsCOMPtr<nsIRunnable>& callback : callbacks) {
> +    callback->Run();

do you care about the error result here?

@@ +2263,5 @@
> +    case State::Completed:
> +    default:
> +      MOZ_CRASH("Bad state!");
> +  }
> +

I think this state machine can be improved. What about if:

struct StateMap {
  State mState;
  bool mMainThread;
} sStateMap[] = {
{ Initial, true },
{ CreatingManager, false },
{ RegisteringObserver, true },
{ CallingCallbacks, false }
};

here you do:
switch (mState) {
  case State::Initial:
    ManageResult(Init());
    break;
  case CreatingManager:
    ManageREsult(CreateManager());
   ...
}

void ManageResult(nsresult rv) {
  if (NS_SUCCEEDED(rv)) {
     mState = sStateMap[mState+1].mState;
     if (sStateMap[mState].mMainThread) {
       Dispatch here or there();
     }
  } else if (mState != CallingCallbacks) {
     mState = CallingCallbacks();
     ...
  }

and you don't change the state in each function.

@@ +4939,5 @@
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this)));
> +  } else {
> +    AdvanceState();
> +
> +    nsresult rv = FinishInit();

Instead calling this directly, can you just do:

AdvanceState();
Run();

@@ +4963,5 @@
> +  AdvanceState();
> +
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mOwningThread->Dispatch(this,
> +                                                       NS_DISPATCH_NORMAL)));
> +

Again, I find this management of the state machine very confusing.
I prefer to have a table of State-Thread and let 1 single function to manage the advanceState plus the dispatching.

::: dom/quota/QuotaManagerService.cpp
@@ +104,5 @@
> +protected:
> +  RefPtr<RequestBase> mRequest;
> +
> +public:
> +  PendingRequestInfo(RequestBase* aRequest)

explicit ?

::: dom/quota/QuotaRequests.cpp
@@ +91,5 @@
> +  return NS_OK;
> +}
> +
> +UsageRequest::UsageRequest(nsIPrincipal* aPrincipal,
> +	                         nsIQuotaUsageCallback* aCallback)

indentation

@@ +161,5 @@
> +
> +NS_IMETHODIMP
> +UsageRequest::GetFileUsage(uint64_t* aFileUsage)
> +{
> +  AssertIsOnOwningThread();

MOZ_ASSERT(aFileUsage);

@@ +184,5 @@
> +
> +NS_IMETHODIMP
> +UsageRequest::SetCallback(nsIQuotaUsageCallback* aCallback)
> +{
> +  AssertIsOnOwningThread();

MOZ_ASSERT(aCallback); ? Do we care?
Attachment #8676875 - Flags: review?(amarchesini) → review+
Attachment #8676880 - Flags: review?(amarchesini) → review+
Comment on attachment 8676881 [details] [diff] [review]
Part 8: Move getFileReferences() from PContent under new protocol PBackgroundIndexedDBUtils

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

::: dom/indexedDB/ActorsChild.cpp
@@ +3101,5 @@
> +
> +  if (mManager) {
> +    mManager->ClearBackgroundActor();
> +#ifdef DEBUG
> +    mManager = nullptr;

why only in DEBUG mode?

::: dom/indexedDB/ActorsParent.cpp
@@ +27203,5 @@
> +GetFileReferencesHelper::DispatchAndReturnFileReferences(int32_t* aMemRefCnt,
> +                                                         int32_t* aDBRefCnt,
> +                                                         int32_t* aSliceRefCnt,
> +                                                         bool* aResult)
> +{

MOZ_ASSERT(all the pointers?)

::: ipc/glue/BackgroundParentImpl.cpp
@@ +197,5 @@
>  auto
> +BackgroundParentImpl::AllocPBackgroundIndexedDBUtilsParent()
> +  -> PBackgroundIndexedDBUtilsParent*
> +{
> +  using mozilla::dom::indexedDB::AllocPBackgroundIndexedDBUtilsParent;

well... I don't see why you want to do it when you can do:
return mozilla::dom::indexedDB::AllocPBackgroundIndexedUtilsParent();

or move it to the other using.

@@ +209,5 @@
> +bool
> +BackgroundParentImpl::DeallocPBackgroundIndexedDBUtilsParent(
> +                                        PBackgroundIndexedDBUtilsParent* aActor)
> +{
> +  using mozilla::dom::indexedDB::DeallocPBackgroundIndexedDBUtilsParent;

ditto.
Attachment #8676881 - Flags: review?(amarchesini) → review+
Comment on attachment 8676882 [details] [diff] [review]
Part 9: Fix asynchronous deletion of files to work on PBackground

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

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +746,5 @@
> +IndexedDatabaseManager::NoteBackgroundThread(nsIEventTarget* aBackgroundThread)
> +{
> +  MOZ_ASSERT(IsMainProcess());
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aBackgroundThread);

MOZ_ASSERT(!mBackgroundThread || mBackgroundThread == aBackgroundThread) ?
Attachment #8676882 - Flags: review?(amarchesini) → review+
Attachment #8676883 - Flags: review?(amarchesini) → review+
do_register_cleanup() is also used by XPCShell test harness to send "profile-before-change" and in this case the notification is sent before quota manager is created (and a profile-before-change observer is registered) so quota manager doesn't have a chance to do cleanup during shutdown, thus some runnables try to run after xpcom shutdown

This particular test actually doesn't need to do its cleanup through registering a cleanup function, so I simply moved the cleanup to the end of the test.
Attachment #8690163 - Flags: review?(amarchesini)
Attachment #8690163 - Flags: review?(amarchesini) → review+
(In reply to Ben Kelly [:bkelly] from comment #8)
> Comment on attachment 8675810 [details] [diff] [review]
> Part 6: Quota Manager on PBackground cache changes
> 
> Review of attachment 8675810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!  I don't see any major issues, just some extra cleanup we can do. 
> r=me with comments.
> 
> ::: dom/cache/Context.cpp
> @@ +378,5 @@
> > +      nsCOMPtr<nsIPrincipal> principal = managerId->Principal();
> > +      nsresult rv = QuotaManager::GetInfoFromPrincipal(principal,
> > +                                                       &mQuotaInfo.mGroup,
> > +                                                       &mQuotaInfo.mOrigin,
> > +                                                       &mQuotaInfo.mIsApp);
> 
> Please file a follow-up bug to remove this GetInfoFromPrincipal().  Instead,
> we should just save all this information when we create the ManagerId:
> 
>   https://dxr.mozilla.org/mozilla-central/source/dom/cache/ManagerId.cpp#27
> 
> This will let us avoid a main thread dispatch.
> 
> Please set the follow-up bug to block bug 1110136.

Done, bug 1226900.

> 
> ::: dom/cache/Manager.cpp
> @@ +248,2 @@
> >  
> > +    // The factory was destroyed between when abort started on main thread and
> 
> Is this comment still accurate?  It seems this can't happen any more because
> its all on the background thread.

Yeah, I remove the comment.

> 
> @@ +314,1 @@
> >      StaticMutexAutoLock lock(sMutex);
> 
> Do we still need sMutex?  It seems everything is running on a single thread
> now.

We actually don't need sBackgroundThread anymore, so I removed it.
(In reply to Andrea Marchesini (:baku) from comment #17)
> Comment on attachment 8676868 [details] [diff] [review]
> Part 2: Remove Utilities.h
> 
> Review of attachment 8676868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/UsageInfo.h
> @@ +81,5 @@
> > +  static void
> > +  IncrementUsage(uint64_t* aUsage, uint64_t aDelta)
> > +  {
> > +    // Watch for overflow!
> > +    if ((UINT64_MAX - *aUsage) < aDelta) {
> 
> This will not work.
> do:
> 
> MOZ_ASSERT(aUsage);
> CheckUint64 value = *aUsage;
> value += aData;
> if (value.isValid()) {
>   *aUsage = value.value();
> } else {
>   *aUsage = UINT64_MAX;
> }

Ok, fixed.
(In reply to Andrea Marchesini (:baku) from comment #20)
> Comment on attachment 8676875 [details] [diff] [review]
> Part 4: QuotaManager on PBackground core changes
> 
> Review of attachment 8676875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me know what you think about this approach of managing the state machine.
> The rest looks good. As usual :)

Ok, thanks. I created a new method GetNextState() for it. I tried the sStateMap, but I think it would be
too complicated/ugly. Enums can't be easily used as array indexes.

> 
> ::: dom/quota/ActorsChild.cpp
> @@ +38,5 @@
> > +
> > +#ifdef DEBUG
> > +
> > +void
> > +QuotaChild::AssertIsOnOwningThread() const
> 
> I have seen this pattern quite often in your code.
> Would be nice to have a base class for this.

Yeah, I created BackgroundThreadObject class for this.

> 
> @@ +57,5 @@
> > +
> > +  if (mService) {
> > +    mService->ClearBackgroundActor();
> > +#ifdef DEBUG
> > +    mService = nullptr;
> 
> any reason to do it only in debug builds?

This is a pattern that is used in indexedDB code, I think we don't need to clear it for the app to work
correctly. It's just for debugging I think.

> 
> @@ +154,5 @@
> > +  AssertIsOnOwningThread();
> > +
> > +  if (mRequest) {
> > +    mRequest->ClearBackgroundActor();
> > +#ifdef DEBUG
> 
> ditto.

See my comment above.

> 
> @@ +204,5 @@
> > +
> > +#ifdef DEBUG
> > +
> > +void
> > +QuotaRequestChild::AssertIsOnOwningThread() const
> 
> yep, definitely would be nice to have a base class :)

This one is a bit special since it forwards to parent protocol implementation and I didn't want to create
super generic and super complicated base class.

> 
> @@ +223,5 @@
> > +  mRequest->SetError(aResponse);
> > +}
> > +
> > +void
> > +QuotaRequestChild::HandleResponse(const RequestResponse& aResponse)
> 
> don't pass this argument if you don't want to use it.
> Are you planning to use this in other patches? If not, maybe a better name
> can be chosen.

Ok, removed the arg, but I kept the name.

> 
> @@ +252,5 @@
> > +    case RequestResponse::TClearOriginResponse:
> > +    case RequestResponse::TClearAppResponse:
> > +    case RequestResponse::TClearAllResponse:
> > +    case RequestResponse::TResetAllResponse:
> > +      HandleResponse(aResponse);
> 
> HandleResponse();

Ok.

> 
> ::: dom/quota/ActorsChild.h
> @@ +38,5 @@
> > +#endif
> > +
> > +public:
> > +  void
> > +  AssertIsOnOwningThread() const
> 
> Yep, a QuotaDebugging class ?

BackgroundThreadObject

> 
> ::: dom/quota/ActorsParent.cpp
> @@ +347,5 @@
> > +
> > +  void
> > +  AddCallback(nsIRunnable* aCallback)
> > +  {
> > +    AssertIsOnOwningThread();
> 
> MOZ_ASSERT(aCallback);

Ok.

> 
> @@ +374,5 @@
> > +
> > +class QuotaManager::ShutdownRunnable final
> > +  : public nsRunnable
> > +{
> > +  bool& mDone;
> 
> // touched only on the main-thread.

Ok.

> 
> @@ +1033,5 @@
> > +  virtual bool
> > +  Init(Quota* aQuota);
> > +
> > +protected:
> > +  QuotaRequestBase(bool aExclusive)
> 
> explicit

Ok.

> 
> @@ +1081,3 @@
> >  };
> >  
> >  class OriginClearOp
> 
> final ?

Ok.

> 
> @@ +2230,5 @@
> > +  nsTArray<nsCOMPtr<nsIRunnable>> callbacks;
> > +  mCallbacks.SwapElements(callbacks);
> > +
> > +  for (nsCOMPtr<nsIRunnable>& callback : callbacks) {
> > +    callback->Run();
> 
> do you care about the error result here?

Added Unused.

> 
> @@ +2263,5 @@
> > +    case State::Completed:
> > +    default:
> > +      MOZ_CRASH("Bad state!");
> > +  }
> > +
> 
> I think this state machine can be improved. What about if:
> 
> struct StateMap {
>   State mState;
>   bool mMainThread;
> } sStateMap[] = {
> { Initial, true },
> { CreatingManager, false },
> { RegisteringObserver, true },
> { CallingCallbacks, false }
> };
> 
> here you do:
> switch (mState) {
>   case State::Initial:
>     ManageResult(Init());
>     break;
>   case CreatingManager:
>     ManageREsult(CreateManager());
>    ...
> }
> 
> void ManageResult(nsresult rv) {
>   if (NS_SUCCEEDED(rv)) {
>      mState = sStateMap[mState+1].mState;
>      if (sStateMap[mState].mMainThread) {
>        Dispatch here or there();
>      }
>   } else if (mState != CallingCallbacks) {
>      mState = CallingCallbacks();
>      ...
>   }
> 
> and you don't change the state in each function.

I did something like this, I hope you're ok with it.

> 
> @@ +4939,5 @@
> > +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this)));
> > +  } else {
> > +    AdvanceState();
> > +
> > +    nsresult rv = FinishInit();
> 
> Instead calling this directly, can you just do:
> 
> AdvanceState();
> Run();

Ok.

> 
> @@ +4963,5 @@
> > +  AdvanceState();
> > +
> > +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mOwningThread->Dispatch(this,
> > +                                                       NS_DISPATCH_NORMAL)));
> > +
> 
> Again, I find this management of the state machine very confusing.
> I prefer to have a table of State-Thread and let 1 single function to manage
> the advanceState plus the dispatching.

Ok.

> 
> ::: dom/quota/QuotaManagerService.cpp
> @@ +104,5 @@
> > +protected:
> > +  RefPtr<RequestBase> mRequest;
> > +
> > +public:
> > +  PendingRequestInfo(RequestBase* aRequest)
> 
> explicit ?

Ok.

> 
> ::: dom/quota/QuotaRequests.cpp
> @@ +91,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +UsageRequest::UsageRequest(nsIPrincipal* aPrincipal,
> > +	                         nsIQuotaUsageCallback* aCallback)
> 
> indentation

Ok.

> 
> @@ +161,5 @@
> > +
> > +NS_IMETHODIMP
> > +UsageRequest::GetFileUsage(uint64_t* aFileUsage)
> > +{
> > +  AssertIsOnOwningThread();
> 
> MOZ_ASSERT(aFileUsage);

Ok.

> 
> @@ +184,5 @@
> > +
> > +NS_IMETHODIMP
> > +UsageRequest::SetCallback(nsIQuotaUsageCallback* aCallback)
> > +{
> > +  AssertIsOnOwningThread();
> 
> MOZ_ASSERT(aCallback); ? Do we care?

Hm, one can also clear the callback, so passing null should be allowed.
(In reply to Andrea Marchesini (:baku) from comment #21)
> Comment on attachment 8676881 [details] [diff] [review]
> Part 8: Move getFileReferences() from PContent under new protocol
> PBackgroundIndexedDBUtils
> 
> Review of attachment 8676881 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/ActorsChild.cpp
> @@ +3101,5 @@
> > +
> > +  if (mManager) {
> > +    mManager->ClearBackgroundActor();
> > +#ifdef DEBUG
> > +    mManager = nullptr;
> 
> why only in DEBUG mode?

I already commented on this.

> 
> ::: dom/indexedDB/ActorsParent.cpp
> @@ +27203,5 @@
> > +GetFileReferencesHelper::DispatchAndReturnFileReferences(int32_t* aMemRefCnt,
> > +                                                         int32_t* aDBRefCnt,
> > +                                                         int32_t* aSliceRefCnt,
> > +                                                         bool* aResult)
> > +{
> 
> MOZ_ASSERT(all the pointers?)

Ok.

> 
> ::: ipc/glue/BackgroundParentImpl.cpp
> @@ +197,5 @@
> >  auto
> > +BackgroundParentImpl::AllocPBackgroundIndexedDBUtilsParent()
> > +  -> PBackgroundIndexedDBUtilsParent*
> > +{
> > +  using mozilla::dom::indexedDB::AllocPBackgroundIndexedDBUtilsParent;
> 
> well... I don't see why you want to do it when you can do:
> return mozilla::dom::indexedDB::AllocPBackgroundIndexedUtilsParent();

Oh, this was just a copy paste. I think it was used to workaround ugly line wrapping.
Fixed.

> 
> or move it to the other using.
> 
> @@ +209,5 @@
> > +bool
> > +BackgroundParentImpl::DeallocPBackgroundIndexedDBUtilsParent(
> > +                                        PBackgroundIndexedDBUtilsParent* aActor)
> > +{
> > +  using mozilla::dom::indexedDB::DeallocPBackgroundIndexedDBUtilsParent;
> 
> ditto.

Ok.
(In reply to Andrea Marchesini (:baku) from comment #22)
> Comment on attachment 8676882 [details] [diff] [review]
> Part 9: Fix asynchronous deletion of files to work on PBackground
> 
> Review of attachment 8676882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +746,5 @@
> > +IndexedDatabaseManager::NoteBackgroundThread(nsIEventTarget* aBackgroundThread)
> > +{
> > +  MOZ_ASSERT(IsMainProcess());
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aBackgroundThread);
> 
> MOZ_ASSERT(!mBackgroundThread || mBackgroundThread == aBackgroundThread) ?

Well, we call this only once currently. I don't think we should change the assertion for now.
(In reply to Jan Varga [:janv] from comment #25)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a1f714dfd1

Ok, try looks good, so if no one objects, I'll try to land it this weekend.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.