Closed Bug 996508 Opened 7 years ago Closed 7 years ago

|nsFrameMessageManager::sPendingSameProcessAsyncMessages| may be processed twice

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → hchang
Blocks: 996510
Attached patch Proposed solution (obsolete) — Splinter Review
Attachment #8406736 - Flags: review?(bugs)
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.
Attachment #8406736 - Flags: review?(bugs) → review-
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.
Whiteboard: [p=1]
Target Milestone: --- → 1.4 S6 (25apr)
Attached patch Propose patch v2 (obsolete) — Splinter Review
Attachment #8406736 - Attachment is obsolete: true
Attachment #8407408 - Flags: review?(bugs)
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
Attachment #8407408 - Flags: review?(bugs) → review+
(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
Attachment #8407408 - Attachment is obsolete: true
Keywords: checkin-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
Flags: needinfo?(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.
Flags: needinfo?(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.
Flags: needinfo?(kwierso)
(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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84aa30627ed3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.