996508 - |nsFrameMessageManager::sPendingSameProcessAsyncMessages| may be processed twice

Status: RESOLVED FIXED

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

Description

[Henry Chang [:hchang]]

For "same process message managers", if the child sends a async message 'A' followed by a sync message 'S', the parent may receive 'A' then 'S' the 'A'.

The following is how things go wrong:

Assume the code looks like:

cpmm.sendAsyncMessage(...);
cpmm.sendSyncMessage(...);

1. Child calling sendAsyncMessage('A') ==> http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameMessageManager.cpp#1819
   |nsAsyncMessageToSameProcessParent('A')| will be (1) created
                                                    (2) pushed to sPendingSameProcessAsyncMessages
                                                    (3) dispatched to current thread

2. Then child calling sendSyncMessage('S') ==> http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameMessageManager.cpp#1796
   Before actually processing 'S', we will check if we have any pending messages. 
   If |sPendingSameProcessAsyncMessages| is not empty, we are trying to process 
   these pending async messages first. So |nsAsyncMessageToSameProcessParent('A')::Run|
   is being called and **|ReceiveMessage|** is being called at this moment.

   However, |nsAsyncMessageToSameProcessParent('A')| has been already dispatched 
   to the current thread and will certainly be called at some time later. 
   When it is called, **|ReceiveMessage|** for 'A' is called AGAIN.

My proposed solution is at |nsAsyncMessageToSameProcessParent('A')::Run|, only when |this| appears in 
|sPendingSameProcessAsyncMessages| will we call |ReceiveMessage|, provided directly working 
on |sPendingSameProcessAsyncMessages| at |SameChildProcessMessageManagerCallback::DoSendingBlockingMessage|.
Applying this solution to the above scenario, when |nsAsyncMessageToSameProcessParent('A')::Run| is called 
first time, |ReceiveMessage| will be called since we could find and remove |nsAsyncMessageToSameProcessParent('A')| in
|sPendingSameProcessAsyncMessages|. The second time |nsAsyncMessageToSameProcessParent::Run| being called, 
|sPendingSameProcessAsyncMessages| has no nsAsyncMessageToSameProcessParent('A') so does nothing.

Comment 1

[Henry Chang [:hchang]]

Attachment: Proposed solution

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8406736 [details] [diff] [review]
Proposed solution

Almost ok, but DoSendBlockingMessage doesn't deal with nested event loops.
You may end up indexing out-of-bounds if some Run() method
spins event loop so that other runnables are removed from
sPendingSameProcessAsyncMessages.

Perhaps the easiest fix is to just add a flag to the message runnable, initially false. Then in Run() check if it is true and return early. Otherwise set to true
and call ReceiveMessage.

Comment 3

[Henry Chang [:hchang]]

You're totally right! I got a crash in certain test case:

https://tbpl.mozilla.org/php/getParsedLog.php?id=37841043&tree=Try#error0

I will be trying the easiest fix you suggested. Thanks!

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 8406736 [details] [diff] [review]
> Proposed solution
> 
> Almost ok, but DoSendBlockingMessage doesn't deal with nested event loops.
> You may end up indexing out-of-bounds if some Run() method
> spins event loop so that other runnables are removed from
> sPendingSameProcessAsyncMessages.
> 
> Perhaps the easiest fix is to just add a flag to the message runnable,
> initially false. Then in Run() check if it is true and return early.
> Otherwise set to true
> and call ReceiveMessage.

Comment 4

[Henry Chang [:hchang]]

Attachment: Propose patch v2

Comment 5

[Henry Chang [:hchang]]

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=f2c14fc7fc4d

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8407408 [details] [diff] [review]
Propose patch v2


>-    nsFrameMessageManager* ppm = nsFrameMessageManager::sSameProcessParentManager;
>-    ReceiveMessage(static_cast<nsIContentFrameMessageManager*>(ppm), ppm);
>+    if (!mDelivered) {
>+      nsFrameMessageManager* ppm = nsFrameMessageManager::sSameProcessParentManager;
>+      ReceiveMessage(static_cast<nsIContentFrameMessageManager*>(ppm), ppm);
>+      mDelivered = true;
>+    }
You need to set mDelivered to true before calling ReceiveMessage, since ReceiveMessage 
may spin the event loop.

With that r=me

Comment 7

[Henry Chang [:hchang]]

Attachment: Propose patch v3 (r+'d)

Comment 8

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #7)
> Created attachment 8412526 [details] [diff] [review]
> Propose patch v3 (r+'d)

Try result of patch v3:

https://hg.mozilla.org/try/rev/1abc03b793af

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out in https://hg.mozilla.org/mozilla-central/rev/6810499bd649 (along with backing out bug 993019) because it seemed like one or both of them made Android J3 tests frequently fail with the error from https://tbpl.mozilla.org/php/getParsedLog.php?id=38639894&tree=Mozilla-Inbound

Comment 11

[Henry Chang [:hchang]]

(In reply to Wes Kocher (:KWierso) from comment #10)
> Backed this out in https://hg.mozilla.org/mozilla-central/rev/6810499bd649
> (along with backing out bug 993019) because it seemed like one or both of
> them made Android J3 tests frequently fail with the error from
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38639894&tree=Mozilla-
> Inbound

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=984596c64344

Only one Android Debug J3 failure with different error from https://tbpl.mozilla.org/php/getParsedLog.php?id=38639894&tree=Mozilla-Inbound.

Comment 12

[Henry Chang [:hchang]]

Another 

https://tbpl.mozilla.org/?tree=Try&rev=734b46b582b8

It seems there's no relevant Android Debug J3 test failure. Is it good for check-in? Thanks.

Comment 13

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #12)
> Another 
> 
> https://tbpl.mozilla.org/?tree=Try&rev=734b46b582b8
> 
> It seems there's no relevant Android Debug J3 test failure. Is it good for
> check-in? Thanks.

Try on today's m-c again:

With patch: https://tbpl.mozilla.org/?tree=Try&rev=311a808bd8b8
w/o patch: https://tbpl.mozilla.org/?tree=Try&rev=c367f6816ef7

No additional test failure caused by the patch. Ask for checkin again. Thanks!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84aa30627ed3

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84aa30627ed3