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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 13 obsolete attachments)

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.
Blocks: 683967
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 ]
Posted patch power monitor base patch (obsolete) — Splinter Review
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.
Assignee: nobody → jmathies
Posted patch base plus win v.1 (obsolete) — Splinter Review
Attachment #573647 - Attachment is obsolete: true
Posted patch mac v.1 (obsolete) — Splinter Review
Posted patch ipc v.1 (obsolete) — Splinter Review
Posted patch base singleton v.1 (obsolete) — Splinter Review
bsmedberg, not sure if you're the right reviewer for this, but you seem to own toolkit and this is a new component, so..
Attachment #578232 - Attachment is obsolete: true
Attachment #578234 - Attachment is obsolete: true
Attachment #578236 - Attachment is obsolete: true
Attachment #578237 - Attachment is obsolete: true
Attachment #578369 - Flags: review?(benjamin)
Posted patch win v.1 (obsolete) — Splinter Review
Posted patch mac v.1 (obsolete) — Splinter Review
Posted patch ipc v.1 (obsolete) — Splinter Review
Posted patch tests update v.1 (obsolete) — Splinter Review
Posted patch tests update v.1 (obsolete) — Splinter Review
Attachment #578373 - Attachment is obsolete: true
Attachment #578375 - Attachment is patch: true
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 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?
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 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.
Attachment #578369 - Flags: review?(benjamin)
No longer blocks: 683967
Posted patch timeout patch v.1 (obsolete) — Splinter Review
Two stage timeout patch. Need to test a bit and push to try.
Attachment #578369 - Attachment is obsolete: true
Attachment #578370 - Attachment is obsolete: true
Attachment #578371 - Attachment is obsolete: true
Attachment #578372 - Attachment is obsolete: true
Attachment #578375 - Attachment is obsolete: true
Posted patch timeout patch v.1 (obsolete) — Splinter Review
Cleaned up with less duplicate code.
Attachment #585874 - Attachment is obsolete: true
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.
Attachment #585887 - Attachment is obsolete: true
Attachment #586038 - Flags: review?(benjamin)
Attachment #586038 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/fdc667b43e11

This landed on Saturday but for some reason didn't get closed out.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 722424
You need to log in before you can comment on or make changes to this bug.