Closed Bug 553606 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Plug-ins, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(3 files, 5 obsolete files)

Attached file 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.
Assignee: nobody → jmathies
Attachment #433569 - Flags: review?(bent.mozilla)
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.
Attachment #433569 - Flags: review?(bent.mozilla)
Attached patch wip v.1 (obsolete) — Splinter Review
Attachment #433569 - Attachment is obsolete: true
Blocks: 554262
Attached patch cleanedup patch (obsolete) — Splinter Review
Attachment #434321 - Attachment is obsolete: true
Attached patch cleanup v.2 (obsolete) — Splinter Review
cleaned up the rpc header a bit.
Attachment #434368 - Attachment is obsolete: true
Comment on attachment 434368 [details] [diff] [review]
cleanedup patch

I think this should be fine... But now I have a headache :(
Attachment #434368 - Flags: review+
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #434374 - Attachment is obsolete: true
Attachment #434448 - Flags: review?(jones.chris.g)
Comment on attachment 434448 [details] [diff] [review]
updated patch

argh, wrong patch.
Attachment #434448 - Flags: review?(jones.chris.g)
Attached patch updated patchSplinter Review
correct patch
Attachment #434448 - Attachment is obsolete: true
Attachment #434449 - Flags: review?(jones.chris.g)
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.
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.)
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.
(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.
Attachment #434449 - Flags: review?(jones.chris.g)
Attached patch patchSplinter Review
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.
http://hg.mozilla.org/mozilla-central/rev/5a1cf671bd61
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.