Closed Bug 808743 Opened 7 years ago Closed 7 years ago

Better protection for PBrowser shutdown and database invalidation in multiprocess scenarios

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(2 files)

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.
This isn't all that complicated, we just need to find all open requests and cancel them.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #678440 - Flags: review?(khuey)
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.
Attachment #678442 - Flags: review?(khuey)
blocking-basecamp: ? → +
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 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.
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
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
https://hg.mozilla.org/mozilla-central/rev/dbbc549daa9e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 799538
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.