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

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 13 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 683967

Comment 1

6 years ago
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 ]
(Assignee)

Comment 2

6 years ago
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.
Assignee: nobody → jmathies
(Assignee)

Comment 3

6 years ago
Created attachment 578232 [details] [diff] [review]
base plus win v.1
Attachment #573647 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Created attachment 578234 [details] [diff] [review]
mac v.1
(Assignee)

Comment 5

6 years ago
Created attachment 578236 [details] [diff] [review]
ipc v.1
(Assignee)

Comment 6

6 years ago
Created attachment 578237 [details] [diff] [review]
obs events and updated tests v.1
(Assignee)

Comment 7

6 years ago
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..
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)
(Assignee)

Comment 8

6 years ago
Created attachment 578370 [details] [diff] [review]
win v.1
(Assignee)

Comment 9

6 years ago
Created attachment 578371 [details] [diff] [review]
mac v.1
(Assignee)

Comment 10

6 years ago
Created attachment 578372 [details] [diff] [review]
ipc v.1
(Assignee)

Comment 11

6 years ago
Created attachment 578373 [details] [diff] [review]
tests update v.1
(Assignee)

Comment 12

6 years ago
Created attachment 578375 [details] [diff] [review]
tests update v.1
Attachment #578373 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #578375 - Attachment is patch: true
(Assignee)

Comment 13

6 years ago
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?
(Assignee)

Comment 15

6 years ago
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)
(Assignee)

Updated

6 years ago
No longer blocks: 683967
(Assignee)

Comment 17

6 years ago
Created attachment 585874 [details] [diff] [review]
timeout patch v.1

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
(Assignee)

Comment 18

6 years ago
Created attachment 585887 [details] [diff] [review]
timeout patch v.1

Cleaned up with less duplicate code.
Attachment #585874 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
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.
Attachment #585887 - Attachment is obsolete: true
Attachment #586038 - Flags: review?(benjamin)
Attachment #586038 - Flags: review?(benjamin) → review+
(Assignee)

Comment 20

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc667b43e11
(Assignee)

Comment 21

6 years ago
https://hg.mozilla.org/mozilla-central/rev/fdc667b43e11

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