Last Comment Bug 679240 - [OOPP] Investigate issues system sleep might have on plugin timeouts
: [OOPP] Investigate issues system sleep might have on plugin timeouts
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: ---
Assigned To: Jim Mathies [:jimm]
:
Mentors:
Depends on: 722424
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-15 17:55 PDT by Jim Mathies [:jimm]
Modified: 2012-01-30 12:05 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
power monitor base patch (48.16 KB, patch)
2011-11-10 14:56 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
base plus win v.1 (25.15 KB, patch)
2011-12-01 05:32 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
mac v.1 (9.26 KB, patch)
2011-12-01 05:32 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
ipc v.1 (11.24 KB, patch)
2011-12-01 05:33 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
obs events and updated tests v.1 (5.67 KB, patch)
2011-12-01 05:34 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
base singleton v.1 (18.36 KB, patch)
2011-12-01 13:31 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
win v.1 (10.07 KB, patch)
2011-12-01 13:31 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
mac v.1 (9.27 KB, patch)
2011-12-01 13:32 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
ipc v.1 (9.67 KB, patch)
2011-12-01 13:33 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
tests update v.1 (9.67 KB, patch)
2011-12-01 13:33 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
tests update v.1 (5.67 KB, patch)
2011-12-01 13:36 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
timeout patch v.1 (4.21 KB, patch)
2012-01-04 13:43 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
timeout patch v.1 (6.71 KB, patch)
2012-01-04 14:07 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
timeout patch v.2 (8.24 KB, patch)
2012-01-05 04:26 PST, Jim Mathies [:jimm]
benjamin: review+
Details | Diff | Review

Description Jim Mathies [:jimm] 2011-08-15 17:55:52 PDT
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#546

Plugin timeouts for both the parent and the child currently don't account for the possibility that a computer might sleep for an extended period during a WaitForNotify call. If this happens we would likely flush all the child processes. This bug is about figuring out what the issues are and hopefully putting together a fix.
Comment 1 u88484 2011-09-09 07:20:36 PDT
I experience this while playing the game 'crime city' on google+.  I leave the game open when putting the computer to sleep and after waking the computer back up I get the plugin crashed notification.  The crash signatures are always [@ mozalloc_abort(char const* const) | msvcr80.dll@0xe456 ]
Comment 2 Jim Mathies [:jimm] 2011-11-10 14:56:06 PST
Created attachment 573647 [details] [diff] [review]
power monitor base patch

Wip of this work. I still have a few things to do- test on mac, break up the patch, and try to work up some tests for this.
Comment 3 Jim Mathies [:jimm] 2011-12-01 05:32:26 PST
Created attachment 578232 [details] [diff] [review]
base plus win v.1
Comment 4 Jim Mathies [:jimm] 2011-12-01 05:32:55 PST
Created attachment 578234 [details] [diff] [review]
mac v.1
Comment 5 Jim Mathies [:jimm] 2011-12-01 05:33:48 PST
Created attachment 578236 [details] [diff] [review]
ipc v.1
Comment 6 Jim Mathies [:jimm] 2011-12-01 05:34:49 PST
Created attachment 578237 [details] [diff] [review]
obs events and updated tests v.1
Comment 7 Jim Mathies [:jimm] 2011-12-01 13:31:02 PST
Created attachment 578369 [details] [diff] [review]
base singleton v.1

bsmedberg, not sure if you're the right reviewer for this, but you seem to own toolkit and this is a new component, so..
Comment 8 Jim Mathies [:jimm] 2011-12-01 13:31:33 PST
Created attachment 578370 [details] [diff] [review]
win v.1
Comment 9 Jim Mathies [:jimm] 2011-12-01 13:32:01 PST
Created attachment 578371 [details] [diff] [review]
mac v.1
Comment 10 Jim Mathies [:jimm] 2011-12-01 13:33:06 PST
Created attachment 578372 [details] [diff] [review]
ipc v.1
Comment 11 Jim Mathies [:jimm] 2011-12-01 13:33:27 PST
Created attachment 578373 [details] [diff] [review]
tests update v.1
Comment 12 Jim Mathies [:jimm] 2011-12-01 13:36:39 PST
Created attachment 578375 [details] [diff] [review]
tests update v.1
Comment 13 Jim Mathies [:jimm] 2011-12-02 08:38:19 PST
bsmedberg, should this also get hooked up to the hang monitor? Looks like we might have a similar problem there if the timeout thread happens to exit it's wait before the event loop starts processing events. Probably not a common occurrence but might still happen occasionally.

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/HangMonitor.cpp#52
Comment 14 Benjamin Smedberg [:bsmedberg] 2011-12-06 10:56:43 PST
Comment on attachment 578369 [details] [diff] [review]
base singleton v.1

I'm torn about this: for the in-process hang monitor we simply wait twice for half the timeout length (e.g. two 15-second waits for a 30-second timeout), which gives you pretty much all of the reliability you need without something quite this complex. See bug 429592 comment 20 for some details.

I've got some minor nitpicks on the patch, but can I get you to comment on the general approach first?
Comment 15 Jim Mathies [:jimm] 2011-12-06 14:57:13 PST
I originally just did up a simple singleton that fired observer events (similar to the old widget code) and added observer support to the channels, and figured that would be it. Then realized child processes didn't have observer service, so had to add the simple callback mechanism. I ended up using that on the channels for both sides of the connection to keep it simple. On chrome side we want this to always run which is conveniently taken care of by the xpcom observer subscription. That's really about it. Platform code should be pretty easy to understand.

The current location (toolkit) I just sort of randomly picked, it seemed like the best place. We now have some hal battery code (just landed and was developed in tandem with this code) that duplicates some of the window/mac code. I plan to file a follow up to blend the two together somehow.
Comment 16 Benjamin Smedberg [:bsmedberg] 2011-12-07 10:28:13 PST
Comment on attachment 578369 [details] [diff] [review]
base singleton v.1

My point is that I don't think we need this code at all for plugin timeouts. Perhaps we need it elsewhere in which case I can review it more thoroughly, but I'd like to try the simpler approach if possible first.
Comment 17 Jim Mathies [:jimm] 2012-01-04 13:43:36 PST
Created attachment 585874 [details] [diff] [review]
timeout patch v.1

Two stage timeout patch. Need to test a bit and push to try.
Comment 18 Jim Mathies [:jimm] 2012-01-04 14:07:53 PST
Created attachment 585887 [details] [diff] [review]
timeout patch v.1

Cleaned up with less duplicate code.
Comment 19 Jim Mathies [:jimm] 2012-01-05 04:26:46 PST
Created attachment 586038 [details] [diff] [review]
timeout patch v.2

Final patch which passes try. I looked for a way to write an ipdl test for this, but didn't see it. Once we're in a Call or Send we don't break out until both stages of the timeout complete, so there doesn't appear to be a way to test the state of mInTimeoutSecondHalf in the middle. We have hang tests already though which test timeouts which passed on try, so things seem to be working as expected.
Comment 21 Jim Mathies [:jimm] 2012-01-11 07:16:57 PST
https://hg.mozilla.org/mozilla-central/rev/fdc667b43e11

This landed on Saturday but for some reason didn't get closed out.

Note You need to log in before you can comment on or make changes to this bug.