Closed Bug 797889 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #668006 - Flags: review?(bent.mozilla)
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.
Attached patch PatchSplinter Review
Comments addressed
Attachment #668006 - Attachment is obsolete: true
Attachment #668006 - Flags: review?(bent.mozilla)
Attachment #668228 - Flags: review?(bent.mozilla)
Attachment #668228 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/72d8054301d2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.