797889 - [IPC] Ensure that we don't send messages to the child process after it has begun to shut down.

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

We need to avoid sending any IPC messages from IndexedDB after shutdown of the relevant parent protocol (PBrowser for in-Window, PContent for out-of-Window).

This patch implements the (hacky) solution we agreed on; having those protocols tell their IndexedDB subprotocols not to send any more messages.  I also refactored the IPC setup for AsyncConnectionHelper slightly to make this cleaner.

The upcoming patch passes all the tests without assertions on the parent or the child side.

Comment 1

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

Attachment: Patch

Comment 2

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

Comment on attachment 668006 [details] [diff] [review]
Patch

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

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +521,5 @@
> +
> +  // If there's no request, there could never have been an actor, and so there
> +  // is nothing to do.
> +  if (!mRequest) {
> +    return Success_NotSent;

Can this happen? I think we should be able to assert that it can't...

@@ +525,5 @@
> +    return Success_NotSent;
> +  }
> +
> +  // All requests that get here have transactions, except for FileHandle,
> +  // which isn't hooked up for IPC yet.

Hm. I don't understand this part. We should chat.

@@ +526,5 @@
> +  }
> +
> +  // All requests that get here have transactions, except for FileHandle,
> +  // which isn't hooked up for IPC yet.
> +  IDBTransaction* trans = mRequest->GetTransaction();

You should be able to use GetCurrentTransaction here without needing to ask the request.

::: dom/indexedDB/AsyncConnectionHelper.h
@@ +125,5 @@
> +  enum ChildProcessSendResult
> +  {
> +    // The result was successfully sent to the child process
> +    Success_Sent = 0,
> +    // The result was not sent, because this is not an out-of-process request.

Nit: Space above here, and between the rest.

@@ +133,5 @@
> +    Success_ActorDisconnected,
> +    // An error occurred.
> +    Error
> +  };
> +  

Nit: Whitespace

::: dom/indexedDB/IDBDatabase.cpp
@@ +293,5 @@
> +  if (owner) {
> +    IndexedDatabaseManager::CancelPromptsForWindow(owner);
> +  }
> +
> +  mStatus |= eDisconnected;

I think you should set this before calling Close(), otherwise we risk reentering.

::: dom/indexedDB/IDBDatabase.h
@@ +196,5 @@
> +    // Used when the database is disconnected from its actor.
> +    eDisconnected = 0x2
> +  };
> +
> +  int32_t mStatus;

I really don't think we need a bitfield here. Two bools should be fine.

::: dom/indexedDB/IDBRequest.h
@@ +83,5 @@
>    void CaptureCaller(JSContext* aCx);
>  
>    void FillScriptErrorEvent(nsScriptErrorEvent* aEvent) const;
>  
> +  IDBTransaction* GetTransaction() const

I'd rather we not add this since AsyncConnectionHelper already knows the current transaction.

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +67,5 @@
> +  const InfallibleTArray<PIndexedDBDatabaseParent*>& dbs =
> +    ManagedPIndexedDBDatabaseParent();
> +
> +  for (uint32_t i = 0; i < dbs.Length(); ++i) {
> +    (static_cast<IndexedDBDatabaseParent*>(dbs[i]))->Disconnect();

Nit: Remove extra parens

@@ +227,5 @@
>  
> +void
> +IndexedDBDatabaseParent::Disconnect()
> +{
> +  mDatabase->DisconnectFromActor();

mDatabase could be null here, need to check.

Also, if the open request is in progress, do we need to do anything to ensure that it disconnects when it completes?

::: dom/indexedDB/ipc/IndexedDBParent.h
@@ +147,5 @@
>    {
>      return mASCIIOrigin;
>    }
>  
> +  void Disconnect();

Nit: void on its own line.

Comment 3

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

Attachment: Patch

Comments addressed

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/72d8054301d2

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/72d8054301d2