679240 - [OOPP] Investigate issues system sleep might have on plugin timeouts

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

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]

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]]

Attachment: 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]]

Attachment: base plus win v.1

Comment 4

[Jim Mathies [:jimm]]

Attachment: mac v.1

Comment 5

[Jim Mathies [:jimm]]

Attachment: ipc v.1

Comment 6

[Jim Mathies [:jimm]]

Attachment: obs events and updated tests v.1

Comment 7

[Jim Mathies [:jimm]]

Attachment: 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]]

Attachment: win v.1

Comment 9

[Jim Mathies [:jimm]]

Attachment: mac v.1

Comment 10

[Jim Mathies [:jimm]]

Attachment: ipc v.1

Comment 11

[Jim Mathies [:jimm]]

Attachment: tests update v.1

Comment 12

[Jim Mathies [:jimm]]

Attachment: tests update v.1

Comment 13

[Jim Mathies [:jimm]]

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]

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]]

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]

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]]

Attachment: timeout patch v.1

Two stage timeout patch. Need to test a bit and push to try.

Comment 18

[Jim Mathies [:jimm]]

Attachment: timeout patch v.1

Cleaned up with less duplicate code.

Comment 19

[Jim Mathies [:jimm]]

Attachment: 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 20

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc667b43e11

Comment 21

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/fdc667b43e11

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