Closed
Bug 996508
Opened 10 years ago
Closed 10 years ago
|nsFrameMessageManager::sPendingSameProcessAsyncMessages| may be processed twice
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
2.0 S1 (9may)
People
(Reporter: hchang, Assigned: hchang)
References
Details
(Whiteboard: [p=1])
Attachments
(1 file, 2 obsolete files)
1.78 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8406736 -
Flags: review?(bugs)
Comment 2•10 years ago
|
||
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-
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=1]
Target Milestone: --- → 1.4 S6 (25apr)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8406736 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Try server result: https://tbpl.mozilla.org/?tree=Try&rev=f2c14fc7fc4d
Assignee | ||
Updated•10 years ago
|
Attachment #8407408 -
Flags: review?(bugs)
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
(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
Assignee | ||
Updated•10 years ago
|
Attachment #8407408 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c65691e01d
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)
Assignee | ||
Comment 11•10 years ago
|
||
(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)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
(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
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/84aa30627ed3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84aa30627ed3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Flags: needinfo?(kwierso)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•