553606 - [OOPP] Limit spin loop to a call depth of one

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Attachment: stack

This hang showed up today while testing. Limit our use of spin loop to one call, so that windowing events on subsequent calls are blocked.

Comment 1

[Jim Mathies [:jimm]]

Attachment: limit spin loop use to a single call depth

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 433569 [details] [diff] [review]
limit spin loop use to a single call depth

not working right, video sites stop playing with this applied.

Comment 3

[Jim Mathies [:jimm]]

Attachment: wip v.1

Comment 4

[Jim Mathies [:jimm]]

Attachment: cleanedup patch

Comment 5

[Jim Mathies [:jimm]]

Attachment: cleanup v.2

cleaned up the rpc header a bit.

Comment 6

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

Comment on attachment 434368 [details] [diff] [review]
cleanedup patch

I think this should be fine... But now I have a headache :(

Comment 7

[Jim Mathies [:jimm]]

Attachment: updated patch

Need to run this past cjones to be sure the call to MaybeProcessDeferredIncall in spin loop (WaitForNotify) is ok. Chris, this added fix fixes bug 554262, which was caused by our new race resolution policy toward paints.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 434448 [details] [diff] [review]
updated patch

argh, wrong patch.

Comment 9

[Jim Mathies [:jimm]]

Attachment: updated patch

correct patch

Comment 10

[Jim Mathies [:jimm]]

I think I'll make an additional change to this. If we some how ended up with an out of turn reply that was still out of turn, this could cause a cpu spike and events wouldn't get processed. So in the final patch I think I'll try moving the |if (!mDeferred.empty())| & |if (!mOutOfTurnReplies.empty())| blocks down below the final Wait.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

jimm, sorry, I've spent about an hour and half reading over this patch, but there are some things I don't quite understand yet wrt bug 554262.  Some initial questions and points though

  - What's the problem with the stack in attachment 433568 [details]?  It appears to me that even if you had received the "break spinloop" message in the nested wait, you would have broken the outer too, right?  (By the IsSpinLoopActive() guard.)

  - Why are sModalCount and sInnerEventCount separate?  Is sModalCount the "number of notifications of children entering modal loops received while already spinning in [some] spinloop"?  If so, does this patch end up being equivalent to breaking out when |#recvd gOOPPSpinNativeLoopEvent == #revd gOOPPStopNativeLoopEvent| (or equivalently, some |sDepth == 0|)?

 - MaybeProcessDeferredIncall() might not process a message even if |mDeferred.size() > 0|.  So it's vulnerable to the same perf issue you point out in comment 10.

(I posted this before bug 554262, but still want to see this resolved.)

Comment 12

[Benjamin Smedberg]

The problem with the stack is that we shouldn't be spinning the internal event loop a second time during the inner RPC call. It could end up processing windows events during painting, for example (which is related to bug 554262 and is a big problem). The only time we should spin the second internal event loop is if the plugin spins another internal event loop, which should be rare.

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #11)
> 
>   - What's the problem with the stack in attachment 433568 [details]?  It appears to me
> that even if you had received the "break spinloop" message in the nested wait,
> you would have broken the outer too, right?  (By the IsSpinLoopActive() guard.)

The spin loop is still active. What I'm doing is is preventing the use of SpinInternalEventLoop for the inner paint. When that completes we'll fall back into spinloop, which is still active. (This is independent of the problem in bug 554262.)
 
> 
>   - Why are sModalCount and sInnerEventCount separate?  Is sModalCount the
> "number of notifications of children entering modal loops received while
> already spinning in [some] spinloop"?  If so, does this patch end up being
> equivalent to breaking out when |#recvd gOOPPSpinNativeLoopEvent == #revd
> gOOPPStopNativeLoopEvent| (or equivalently, some |sDepth == 0|)?

EnterModalLoop/ExitModalLoop w/sInnerEventLoopDepth track that calls into SpinInternalEventLoop. I should probably change the names of those to Enter/ExitSpinLoop to make this clearer. As bsmedberg points out in his comment, we can have multiple modal loops going, so technically the number of possible calls to SpinInternalEventLoop must be equal to the number of blocked IPC out-calls that have dropped into modal loops in the child. That's what IncModalLoopCnt/DecModalLoopCnt w/sModalEventCount track, and WaitNeedsSpinLoop does the math to figure out if we need to use SpinInternalEventLoop or if we can fall into WaitForNotify processing. 

> 
>  - MaybeProcessDeferredIncall() might not process a message even if
> |mDeferred.size() > 0|.  So it's vulnerable to the same perf issue you point
> out in comment 10.
> 
> (I posted this before bug 554262, but still want to see this resolved.)

Your patch there applies a similar fix to address the deferred message problem in the main rpc channel code, so I think I can take out my changes. Let me test with that and my previous patch to make sure the problem in bug 554262 is still addressed.

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch

Ok, after pulling my deferred message processing out, applying the patch in bug 554262 plus the one change I mentioned in the review, the problem with the phone site is fixed. Here's the previous patch which bent reviewed. I made one change, I renamed EnterModalLoop/ExitModalLoop to EnterSpinLoop/ExitSpinLoop.

Comment 15

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/5a1cf671bd61

Comment 16

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/ccd9b0bae32a

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 18

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430