808743 - Better protection for PBrowser shutdown and database invalidation in multiprocess scenarios

Status: VERIFIED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Due to the two-step shutdown process for PBrowser and problems with database invalidation in multiprocess scenarios we can end up with either the child or parent process asserting or aborting. Attached patches fix all the races I was able to find.

Comment 1

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

Attachment: Child changes, v1

This isn't all that complicated, we just need to find all open requests and cancel them.

Comment 2

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

Attachment: Parent changes, v1

This one is harder.

Basically we can't allow any messages to be sent to the child process through PIndexedDB or subprotocols after PBrowser::SendDestroy has been called. In order to enforce this more generically I switched our parent actors to use private (or, when needed, protected) inheritance of their protocol bases. This way we can ensure that no other code outside IndexedDBParent can send messages, and we can make sure to check the disconnected status there.

The remaining changes work around the fact that, for instance, IndexedDBParent no longer casts to PIndexedDBParent.

Comment 3

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

Comment on attachment 678440 [details] [diff] [review]
Child changes, v1

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

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +208,5 @@
> +      case Success_Sent: {
> +        if (mRequest) {
> +          mRequest->NotifyHelperSentResultsToChildProcess(NS_OK);
> +        }
> +      } break;

I think break inside the braces is more consistent with our other code.

@@ +239,5 @@
> +        }
> +      } break;
> +
> +      default:
> +        NS_NOTREACHED("Unknown value for ChildProcessSendResult!");

MOZ_NOT_REACHED will abort in opt builds.  Should probably use that instead.

::: dom/indexedDB/IDBRequest.h
@@ +86,5 @@
>  
> +  bool IsPending() const
> +  {
> +    return !mHaveResultOrErrorCode;
> +  }

You should change IDBRequest::GetReadyState to use this function.

::: dom/indexedDB/IDBTransaction.cpp
@@ +224,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +  NS_ASSERTION(mPendingRequests, "Mismatched calls!");
> +  --mPendingRequests;
> +}

I think we should preserve the invariant that we always call CommitOrRollback and modify it to deal with the disconnect case.

Comment 4

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

Comment on attachment 678442 [details] [diff] [review]
Parent changes, v1

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

::: dom/indexedDB/IDBIndex.cpp
@@ -2309,4 @@
>          return Error;
>        }
> -
> -      openCursorResponse = cursorActor;

Hmm, this leaked before didn't it?

::: dom/indexedDB/IDBObjectStore.cpp
@@ +2561,5 @@
>    }
>  
> +  // If we've been invalidated then there's no point sending anything to the
> +  // parent process.
> +  if (mObjectStore->Transaction()->Database()->IsInvalidated()) {

That's a mouthful :-P

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +157,5 @@
> +  }
> +
> +  if (!mFactory) {
> +    return true;
> +  }

Hrm, why can't we assert after doing the disconnection check?

@@ +341,5 @@
> +{
> +  MOZ_ASSERT(mDatabase);
> +
> +  if (!IsDisconnected()) {
> +    mozilla::unused << SendInvalidate();

Ugh I hate mozilla::unused.

@@ +1040,5 @@
> +      return actor->Continue(aParams.get_ContinueParams());
> +
> +    default:
> +      MOZ_NOT_REACHED("Unknown type!");
> +      return false;

I'm pretty sure you don't need the return, MOZ_NOT_REACHED you do the builtin-unreachable stuff.

@@ +1043,5 @@
> +      MOZ_NOT_REACHED("Unknown type!");
> +      return false;
> +  }
> +
> +  MOZ_NOT_REACHED("Should never get here!");

Here too.

::: dom/indexedDB/ipc/IndexedDBParent.h
@@ +466,5 @@
> +  IsDisconnected() const
> +  {
> +    IndexedDBTransactionParent* manager =
> +      static_cast<IndexedDBTransactionParent*>(Manager());
> +    return manager->IsDisconnected();

Why use the intermediate variable (unlike all the others)?

@@ +549,5 @@
> +  IsDisconnected() const
> +  {
> +    IndexedDBVersionChangeTransactionParent* manager =
> +      static_cast<IndexedDBVersionChangeTransactionParent*>(Manager());
> +    return manager->IsDisconnected();

And here.  We should be consistent.

@@ +662,3 @@
>  protected:
> +  // Don't let anyone call this directly, instead go through SendResponse.
> +  using PIndexedDBRequestParent::Send__delete__;

Hmm, doesn't this mean that

IndexedDBParentRequestBase* requestBase = GetIt();
requestBase->Send__delete__(response); // Fails to compile
static_cast<PIndexedDBRequestParent*>(requestBase)->Send__delete__(response); // Successfully compiles

Can we make this inherit privately too?

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +770,4 @@
>      nsContentUtils::GetParserService()->
>      IsBlock(nsContentUtils::GetParserService()->HTMLAtomTagToId(tagAtom),
>              isBlock);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

This has nothing to do with this patch?

::: ipc/chromium/src/chrome/common/ipc_channel_win.cc
@@ +108,5 @@
>  
>  bool Channel::ChannelImpl::Send(Message* message) {
> +  if (thread_check_.get()) {
> +    DCHECK(thread_check_->CalledOnValidThread());
> +  }

If this is part of this patch cjones or someone should review.

::: ipc/ipdl/ipdl/lower.py
@@ +2859,3 @@
>                  managertype = p.managerActorType(self.side, ptr=1)
>                  managermeth = MethodDefn(MethodDecl(
> +                    p.managerMethod().name, ret=managertype, const=1))

Same.

Comment 5

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

khuey and I talked about most of this stuff in person. cjones stamped the ipc changes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbbc549daa9e

Comment 6

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> I think we should preserve the invariant that we always call
> CommitOrRollback and modify it to deal with the disconnect case.

We decided that this wasn't a good idea.

Comment 7

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> I'm pretty sure you don't need the return, MOZ_NOT_REACHED you do the
> builtin-unreachable stuff.

This is true, but I realized I have this pattern all over the place. Best to do it in a separate cleanup bug.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dbbc549daa9e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6157d5ec9eb7

Comment 11

[John Hammink]

Used the steps in https://bugzilla.mozilla.org/show_bug.cgi?id=799538 to test this case.   Verified fixed on 11/14 nightly - although there are other stability issues with dialer on extended DTMF sessions.