Closed
Bug 1178890
Opened 9 years ago
Closed 9 years ago
TimerThread::DoAfterSleep() seems to not be threadsafe
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox38 | --- | wontfix |
firefox38.0.5 | --- | wontfix |
firefox39 | --- | wontfix |
firefox40 | + | verified |
firefox41 | + | fixed |
firefox42 | + | fixed |
firefox-esr31 | --- | wontfix |
firefox-esr38 | 40+ | verified |
b2g-v2.0 | --- | fixed |
b2g-v2.0M | --- | fixed |
b2g-v2.1 | --- | fixed |
b2g-v2.1S | --- | fixed |
b2g-v2.2 | --- | fixed |
b2g-v2.2r | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: bwc, Assigned: jesup)
References
Details
(Keywords: sec-high, Whiteboard: [adv-main40+][adv-esr38.2+])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bwc
:
review+
froydnj
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr31-
Sylvestre
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
TimerThread::DoAfterSleep() iterates over mTimers without any locking, updating timeouts. Any insertions of new timers (or canceling/rescheduling) is likely to go awry if DoAfterSleep is running.
I need to verify that this function is ever called, since dxr doesn't seem to know about any callers.
Comment 1•9 years ago
|
||
Presumably the call you are looking for is:
http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#514
?
Reporter | ||
Comment 2•9 years ago
|
||
Right, but dxr doesn't know about callers of |Observe|.
Reporter | ||
Comment 3•9 years ago
|
||
Verified that |DoAfterSleep| is indeed called when coming out of sleep. This is an actual problem. This could be the root cause for bug 1151046 (since this could cause timers to be inserted in the wrong order).
Reporter | ||
Comment 4•9 years ago
|
||
Calling this sec-high, since this could easily lead to a UAF of the buffer that backs mTimers, if a re-allocation happens due to an insertion while DoAfterSleep is running. It could also trip up code that relies on timers firing in a deterministic order.
Keywords: sec-high
Reporter | ||
Comment 5•9 years ago
|
||
This just gets better and better. The call to timer->SetDelay() in DoAfterSleep eventually leads here:
https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#387
There are a few things worth noting here:
1. We lock in here, which is going to make fixing this bug a little more complex.
2. We remove the timer from mTimers, and re-insert it, while iterating over mTimers (yikes)
3. This code assumes the caller is holding onto a strong ref to the timer (it isn't; also yikes)
Reporter | ||
Comment 6•9 years ago
|
||
It seems like it might be prudent to lock, copy mTimers into a temporary, clear mTimers, unlock, and then reschedule everything.
Assignee | ||
Comment 7•9 years ago
|
||
This is sounding more like a sec-crit given the UAF potential and the possibility it could cause revectoring of calls. Thanks for digging in, Byron. I agree this may explain bug 1151046, of which analysis has not yielded any way the timers could be out-of-order.
NI dveditz on the sec level (or froyd may want to opine)
Likely affects all versions and ESRs
Severity: normal → critical
Flags: needinfo?(dveditz)
Reporter | ||
Comment 8•9 years ago
|
||
Ok, the more I think about this the weirder it becomes. On wake from sleep, we are trying to re-schedule all existing timers why? For example, if I schedule a timer to fire in one minute, and just before this timer pops the machine goes to sleep, why should it be re-scheduled for one minute after the machine comes out of sleep? Shouldn't it fire right away? Put another way, if I schedule a timer (A) for one minute, and 50 seconds later I schedule a timer (B) for 30 seconds, the expected order of firing is A then B. But if the machine goes to sleep, after waking the order will be B then A!
Reporter | ||
Comment 9•9 years ago
|
||
(This is assuming the algorithm works; we're mutating mTimers while iterating over it so it probably doesn't, but this does seem to be the intent)
Reporter | ||
Comment 10•9 years ago
|
||
Do you have any idea why it might be a good idea to reschedule timers like described in comment 8?
Flags: needinfo?(khuey)
(In reply to Byron Campen [:bwc] from comment #8)
> Ok, the more I think about this the weirder it becomes. On wake from sleep,
> we are trying to re-schedule all existing timers why? For example, if I
> schedule a timer to fire in one minute, and just before this timer pops the
> machine goes to sleep, why should it be re-scheduled for one minute after
> the machine comes out of sleep? Shouldn't it fire right away? Put another
> way, if I schedule a timer (A) for one minute, and 50 seconds later I
> schedule a timer (B) for 30 seconds, the expected order of firing is A then
> B. But if the machine goes to sleep, after waking the order will be B then A!
We're not rescheduling them for the unelapsed time, but rather the original time interval? That does seem crazy.
Flags: needinfo?(khuey)
Reporter | ||
Comment 12•9 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> (In reply to Byron Campen [:bwc] from comment #8)
> > Ok, the more I think about this the weirder it becomes. On wake from sleep,
> > we are trying to re-schedule all existing timers why? For example, if I
> > schedule a timer to fire in one minute, and just before this timer pops the
> > machine goes to sleep, why should it be re-scheduled for one minute after
> > the machine comes out of sleep? Shouldn't it fire right away? Put another
> > way, if I schedule a timer (A) for one minute, and 50 seconds later I
> > schedule a timer (B) for 30 seconds, the expected order of firing is A then
> > B. But if the machine goes to sleep, after waking the order will be B then A!
>
> We're not rescheduling them for the unelapsed time, but rather the original
> time interval? That does seem crazy.
Yeah. At first I thought that this might be doing something reasonable like making the time during sleep "not count", but it is just straight up rescheduling everything for now + delay (except for repeating precise, in which case it is scheduled for <old-timeout> + delay, which is also weird).
Reporter | ||
Comment 13•9 years ago
|
||
I'm going to rip out this rescheduling business and do a try push, and see what happens.
Reporter | ||
Comment 14•9 years ago
|
||
Assignee | ||
Comment 15•9 years ago
|
||
I would expect some form of either unexpired-time, or reschedule with sleep-time removed (though that may cause bad amounts of firing all at once coming out of sleep). Reschedule with Original time is just plain wacky.
If it was decrementing 'delay' as it goes along then it would be unexpired-time - though there still could be vulnerabilities.
BTW,the 1 minute and then 50-seconds later add a 30-second timer case would be racy unless you have some other way to *know* >30 seconds had elapsed before scheduling it (which one could, but it opens the question of timer accuracy - generally timers are a form of "at or after". Cross-timer invariants generally involve 0-timeout timers or "B will happen after A if B has an equal-or-longer delay than A and is added after A". If B has shorter than A, then I'm not sure when of if the invariant applies (without more thought or doc/code-reading).
Comment 16•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #7)
> This is sounding more like a sec-crit given the UAF potential
Do we have a reasonably reliable way to trigger these crashes from external content? If not sec-high is fine for racy pitfalls. Then again if user content can directly cause timer creation (setTimeout?) then maybe that is enough to play with that we should just assume sec-critical. I'm fine either way, and either way we'll definitely backport this to ESR.
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.1S:
--- → affected
status-b2g-v2.2:
--- → affected
status-b2g-v2.5:
--- → affected
status-b2g-master:
--- → affected
status-firefox38:
--- → wontfix
status-firefox38.0.5:
--- → wontfix
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox-esr31:
--- → affected
status-firefox-esr38:
--- → affected
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
Flags: needinfo?(dveditz)
Reporter | ||
Comment 17•9 years ago
|
||
Try run looks basically ok...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d8dbee51043
Assignee | ||
Comment 18•9 years ago
|
||
The problem is that try runs don't sleep.... This may require manual testing.
Reporter | ||
Comment 19•9 years ago
|
||
Absolutely. Question is, what should I test? This effects a lot more than just webrtc.
Reporter | ||
Comment 20•9 years ago
|
||
In particular, I worry that there is code out there that depends on us not firing a bunch of old timers all at once when something comes out of sleep.
Assignee | ||
Comment 21•9 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #20)
> In particular, I worry that there is code out there that depends on us not
> firing a bunch of old timers all at once when something comes out of sleep.
Exactly. And even if it doesn't depend on it, it could cause real annoyance to users/sites. I think we need to account for remaining time if at all possible, and *specify* the invariants.
I think you'll need to craft a test that abuses the timer API and checks the invariants constantly, and sleep/wake-up many times. We should get QA to pound on this after vetting the test program. And test normal operation.
Reporter | ||
Comment 22•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #21)
> (In reply to Byron Campen [:bwc] from comment #20)
> > In particular, I worry that there is code out there that depends on us not
> > firing a bunch of old timers all at once when something comes out of sleep.
>
> Exactly. And even if it doesn't depend on it, it could cause real annoyance
> to users/sites. I think we need to account for remaining time if at all
> possible, and *specify* the invariants.
>
> I think you'll need to craft a test that abuses the timer API and checks the
> invariants constantly, and sleep/wake-up many times. We should get QA to
> pound on this after vetting the test program. And test normal operation.
Accounting for remaining time seems reasonable, and is definitely way more reasonable than what we're doing now. I'll have to think about what I would put in such a test-case.
Assignee | ||
Comment 23•9 years ago
|
||
First cut idea: schedule a bunch of random short timers (and when the fire, schedule new short timers). link each one to the list of timers that the invariants say should not fire before it. Include a non-0 chance that we select 0 for a timeout, and that we schedule multiple 0 timeouts in the same JS sequence (to check the invariant that scheduling two 0-timeout timers will expire in order always). Start this, then sleep/wakeup/repeat ad nauseum until the tester gets tired. For extra points (but not as the only test, since it single-cores things!) do it under rr. And then repeat on different OS's.
The actual testing (once you verify the test works) can be done by Juan or perhaps better softvision.
Reporter | ||
Comment 24•9 years ago
|
||
We'll need multiple threads scheduling timers. To do this with JS will require workers I guess, and a human causing sleep. It would be vastly better to have a test-case in c++ land.
Assignee | ||
Comment 25•9 years ago
|
||
Let's get a fix now, and then deal with the test/verification. I think we know there's a problem now that can lead to a UAF (not to mention the functionality issues that may have caused other problems).
We could write a simple testcase that invokes one of the problematic issues waking from sleep.
Assignee | ||
Comment 26•9 years ago
|
||
First-cut fix proposal: (to avoid this drifting)
a) add locking (yes, this means some extra overhead in timers)
b) if we know the sleep-time delta (got pre-sleep notification), then adjust all timers forward by delta (and ensure that the shortest timer ends up >= 0; if not adjust all forward such that it has a 0 timeout). Retain ordering. This effectively attempts to reschedule based on "remaining time" as best we can tell.
c) if we don't have a sleep-time delta (no pre-sleep time), adjust all timers by a delta based on the last timer that fired (or was added) before the sleep used as the pre-sleep time, and process as per 'b' above. This may be over-aggressive about increasing timeouts as it'll be an over-estimate of slept time (though likely they'll be close to correct).
This should be safe/low-risk. (Note that some timeout-based things may still fail after wakeup (like timed re-registration); this doesn't attempt to solve such an issue, but it does reduce the impact of such an issue somewhat. To 'solve' such an issue would require exposing "wakeup" events to content so it knows to re-register/etc.)
After landing this we can look at options with lower overhead (minimize/avoid locks, etc). But right now it's unsafe, and can cause incorrect firing orders and break things.
Nathan: how do you want to proceed? Comments from anyone else/Byron?
Flags: needinfo?(nfroyd)
Reporter | ||
Comment 27•9 years ago
|
||
I'm working on a timer fuzz test in TestTimers. Seeing some unexpected behavior without any sleep business, trying to get to the bottom of it. Might spawn additional sec bugs.
Comment 28•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #26)
> First-cut fix proposal: (to avoid this drifting)
>
> a) add locking (yes, this means some extra overhead in timers)
The plans for adjusting the firing time of timers seems straightforward enough, though I'm a bit curious about whether there are races between timers going off once we come out of sleep and those same timers being adjusted forward...maybe we just have to accept that...
It's not clear to me what "add locking" means--what are you locking, when are you locking it, etc. My immediate assumption is locking the manipulation of timers we do once we come out of sleep, but then I don't see how that adds the extra overhead in timers that you mention. Can you elaborate on this part of the plan, or point out where I am missing things?
Flags: needinfo?(nfroyd)
Reporter | ||
Comment 29•9 years ago
|
||
We would need to lock TimerThread::mMonitor for the duration of timer adjustment. Given that this is simply a matter of a single integer addition for each scheduled timer, and these timers are stored in an array, this should not introduce much contention with other threads trying to schedule/cancel/etc timers.
Assignee | ||
Comment 30•9 years ago
|
||
I'm working on a patch for this
Assignee | ||
Comment 31•9 years ago
|
||
Note that we *never* fire sleep/wake/suspend/resume except on Windows.... Doing a windows build to try to 5Dtest there
Attachment #8631220 -
Flags: review?(nfroyd)
Attachment #8631220 -
Flags: review?(docfaraday)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Reporter | ||
Comment 32•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #31)
> Created attachment 8631220 [details] [diff] [review]
> Update timer arrays after sleep to account for time sleeping
>
> Note that we *never* fire sleep/wake/suspend/resume except on Windows....
> Doing a windows build to try to 5Dtest there
I checked on OS X, and DoAfterSleep() is called there too.
Assignee | ||
Comment 33•9 years ago
|
||
Aha, hiding in widget/cocoa/nsToolkit.mm (search macros bit me)
Reporter | ||
Comment 34•9 years ago
|
||
Comment on attachment 8631220 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping
Review of attachment 8631220 [details] [diff] [review]:
-----------------------------------------------------------------
There's some simplification and threadsafety work that I want to see here.
::: xpcom/threads/TimerThread.cpp
@@ +30,5 @@
> mShutdown(false),
> mWaiting(false),
> mNotified(false),
> + mSleeping(false),
> + mLastTimerFiredAt(TimeStamp::Now())
I would make this mLastTimeEventLoopRan (or similar), and update it here: https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp?from=TimerThread.cpp#244
@@ +484,5 @@
>
> void
> TimerThread::DoBeforeSleep()
> {
> + mStartedSleepAt = TimeStamp::Now();
I don't think it is necessary to have a second variable if we're remembering the last time the event loop ran; that should be more than good enough. Nobody expects high precision when coming out of sleep. Also, I'm not sure what thread this is called from, but I would be surprised if it were the TimerThread. Lastly, removing this extra variable simplifies the logic in DoAfterSleep considerably.
@@ +502,5 @@
> + // a small over-estimate
> + slept = now - mStartedSleepAt;
> + } else {
> + // an over-estimate of time slept, usually small
> + slept = now - mLastTimerFiredAt;
I'm not totally sure what thread this ends up being called from; I suspect it is not the TimerThread. Since we need to lock mMonitor anyway to update the timeouts, we can just protect ourselves with that.
@@ +516,4 @@
> for (uint32_t i = 0; i < mTimers.Length(); i ++) {
> nsTimerImpl* timer = mTimers[i];
> + if (timer->mTimeout + slept < now) {
> + slept = now - timer->mTimeout;
Expired timers are not all that unusual, so I don't really see the harm. Also, if we just use the last time we ran the event loop, we should never have a timer that satisfies timer->mTimeout + slept < now.
@@ +526,5 @@
> + nsTimerImpl* timer = mTimers[i];
> + timer->mTimeout += slept;
> + }
> + mSleeping = false;
> + mLastTimerFiredAt = now;
Probably not necessary? I guess there's no harm.
@@ +531,4 @@
>
> + // Wake up the timer thread to process the updated array
> + mNotified = true;
> + mMonitor.Notify();
Good idea.
Attachment #8631220 -
Flags: review?(docfaraday) → review-
Assignee | ||
Comment 35•9 years ago
|
||
I verified the original patch on Windows and the 'slept' adjustment looks reasonable. Added a couple of assertions about the monitor being held; those aren't necessary, but if we think the perf impact of them is small (likely, though maybe this is expensive on windows?) in debug builds we should keep them.
Attachment #8631596 -
Flags: review?(nfroyd)
Attachment #8631596 -
Flags: review?(docfaraday)
Assignee | ||
Updated•9 years ago
|
Attachment #8631220 -
Attachment is obsolete: true
Attachment #8631220 -
Flags: review?(nfroyd)
Reporter | ||
Comment 36•9 years ago
|
||
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping
Review of attachment 8631596 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, only would like to see one small change.
::: xpcom/threads/TimerThread.cpp
@@ +479,5 @@
> TimerThread::ReleaseTimerInternal(nsTimerImpl* aTimer)
> {
> + if (!mShutdown) {
> + // copied to a local array before releasing in shutdown
> + mMonitor.AssertCurrentThreadOwns();
Yikes. Sometimes we call this with a lock, sometimes we don't... I'll see if this can be cleaned up.
@@ +507,1 @@
> mSleeping = true; // wake may be notified without preceding sleep notification
Since we're locking now, I don't think anything will be able to observe this transition, so it can probably go away.
Attachment #8631596 -
Flags: review?(docfaraday) → review+
Comment 37•9 years ago
|
||
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping
Review of attachment 8631596 [details] [diff] [review]:
-----------------------------------------------------------------
WFM. I think removing mSleeping is safe, but I'd have to think about it a little bit, and I'd prefer it happen in a separate patch, probably in a different bug.
::: xpcom/threads/TimerThread.cpp
@@ +500,5 @@
> TimerThread::DoAfterSleep()
> {
> + // Mainthread
> + TimeStamp now = TimeStamp::Now();
> + TimeDuration slept;
Code nit: I think this declaration should be moved down and combined with the initialization of |slept|, below.
Attachment #8631596 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 38•9 years ago
|
||
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Hard or very hard, quite possibly extremely hard/impossible - and requires sleep and wakeup to even have a chance
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Clearly adds locking and monitor checks. The rest of the patch has a clear purpose (avoiding timer reordering, which has limited to no direct security implications, so that makes it less obvious.
Which older supported branches are affected by this flaw?
All - code predates VCS->mercurial
If not all supported branches, which bug introduced the flaw?
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply trivially, or be trivial to merge.
How likely is this patch to cause regressions; how much testing does it need?
Overall low risk, though in a critical piece. Changes worth noting though are only in the Wakeup path. A test rig (c++ unit test perhaps) to exercise this and the wake/sleep code might help; otherwise minor manual testing on mac and Windows (no sleep/wake notification on linux it appears). It's unlikely to be worse than the current code.
Given the long history of this flaw, we could ride the trains, or bake on nightly for a while before uplift.
Attachment #8631596 -
Flags: sec-approval?
Comment 39•9 years ago
|
||
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping
sec-approval+. We'll want branch patches, including ESR.
Attachment #8631596 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
Assignee | ||
Comment 40•9 years ago
|
||
Target Milestone: --- → mozilla42
Comment 41•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 42•9 years ago
|
||
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Possible UAF (unproven but believable) in timer code
User impact if declined:
Low if no one has managed to exploit it yet (or after it shows up in the tree). Also can only happen during sleep/wakeup.
Fix Landed on Version:
42
Risk to taking this patch (and alternatives if risky):
Pretty low risk for a core xpcom change, and limited to Wakeup. Grabs the lock (duh) and actually simplified the code a bunch.
String or UUID changes made by this patch:
none
Attachment #8631596 -
Flags: approval-mozilla-esr38?
Attachment #8631596 -
Flags: approval-mozilla-esr31?
Attachment #8631596 -
Flags: approval-mozilla-beta?
Attachment #8631596 -
Flags: approval-mozilla-aurora?
Comment 43•9 years ago
|
||
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping
sec-high, taking it.
However, not on esr31 as we won't be doing a new esr (except in case of chemspill).
Attachment #8631596 -
Flags: approval-mozilla-esr38?
Attachment #8631596 -
Flags: approval-mozilla-esr38+
Attachment #8631596 -
Flags: approval-mozilla-esr31?
Attachment #8631596 -
Flags: approval-mozilla-esr31-
Attachment #8631596 -
Flags: approval-mozilla-beta?
Attachment #8631596 -
Flags: approval-mozilla-beta+
Attachment #8631596 -
Flags: approval-mozilla-aurora?
Attachment #8631596 -
Flags: approval-mozilla-aurora+
Comment 44•9 years ago
|
||
Comment 45•9 years ago
|
||
Comment 46•9 years ago
|
||
Comment 47•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #23)
> The actual testing (once you verify the test works) can be done by Juan or
> perhaps better softvision.
Softvision can help here if you still need it and have a test at hand. If you do, then please give us some details on: test steps, expected bad behavior and expected fixed behavior.
Flags: needinfo?(rjesup)
Assignee | ||
Comment 48•9 years ago
|
||
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #47)
> (In reply to Randell Jesup [:jesup] from comment #23)
> > The actual testing (once you verify the test works) can be done by Juan or
> > perhaps better softvision.
>
> Softvision can help here if you still need it and have a test at hand. If
> you do, then please give us some details on: test steps, expected bad
> behavior and expected fixed behavior.
We don't have a test in hand, the bug was found by inspection. Byron is working on a way to test this and other aspects of the timer API, but that's a complex task.
A basic functionality test would be to set up something that will timeout in a minute (and display some text on the page, or whatever), then put the machine to sleep for a few minutes, and wake it up and see if the timer fires more or less when it should (with a 2 minute timer, and you put it to sleep after 30 seconds, it should fire around 1:30 after waking up - note that time will start once you wake the machine up, not when you log in. This merely verifies we didn't break anything major. (I did smoketests like this before landing.)
This will only be seen on Windows and Mac; we don't have any direct support for sleep notification on linux
Flags: needinfo?(rjesup) → needinfo?(florin.mezei)
Comment 49•9 years ago
|
||
Thanks Randell! Setting as qe-verify+.
I'm assuming a simple example to use for the basic functionality test would be something like this: http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_settimeout. Or more similar examples like this: http://www.w3schools.com/jsref/met_win_settimeout.asp.
Flags: needinfo?(florin.mezei) → qe-verify+
Assignee | ||
Comment 50•9 years ago
|
||
> I'm assuming a simple example to use for the basic functionality test would
> be something like this:
> http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_settimeout.
> Or more similar examples like this:
> http://www.w3schools.com/jsref/met_win_settimeout.asp.
yes
Comment 51•9 years ago
|
||
Comment 52•9 years ago
|
||
Comment 53•9 years ago
|
||
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #49)
> Thanks Randell! Setting as qe-verify+.
>
> I'm assuming a simple example to use for the basic functionality test would
> be something like this:
> http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_settimeout.
> Or more similar examples like this:
> http://www.w3schools.com/jsref/met_win_settimeout.asp.
I used this examples and the steps provided by Randell from comment 48 with Firefox 40 beta 6 and latest ESR 38.1.0 tinderbox build. The alert appears exactly after the time I set, after waking up the PC from sleep. I used Windows 7 64-bit and Mac OS X 10.10.4 OS`s.
Assignee | ||
Comment 54•9 years ago
|
||
There have been reports on Yammer of beachballing coming out of sleep on Macs recently. Since this has recently changed and affects sleep, I want to take a close look at these reports. It may be coincidence.
Comment 55•9 years ago
|
||
FWIW we get bouts of people reporting hangs after sleep occasionally, stretching back for years.
Assignee | ||
Comment 56•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #55)
> FWIW we get bouts of people reporting hangs after sleep occasionally,
> stretching back for years.
Hmmm. Who knows, maybe this will help that! Before this patch, timers could end up out-of-order after sleep. If it's not a recent regression (and IIRC sheppy thought it was), then it's not me. Fingers crossed. (though this seems like a pretty clean patch). If your process vm size is especially large, that might cause fun on wakeup
Updated•9 years ago
|
Flags: qe-verify+
Updated•9 years ago
|
Whiteboard: [adv-main40+][adv-esr38.2+]
Comment 57•9 years ago
|
||
byron - awesome to hear this is fixed. thanks!
I have sort of a drive by thought - over in
https://bugzilla.mozilla.org/show_bug.cgi?id=1152048 we found that NS_WIDGET_WAKE_OBSERVER_TOPIC would call observers re-entrantly (emerging from sleep). That was causing a deadlock in socket transport (fixed)
https://bugzilla.mozilla.org/show_bug.cgi?id=1186074 was filed to notify the observers asynchronously from the wake.
According to my really quick drive by -it looks like the timer code could also have this problem, and the new locks added here might actually deadlock on it (though there was always a correctness problem).
Flags: needinfo?(docfaraday)
Reporter | ||
Comment 58•9 years ago
|
||
I think the key problem here was the call to nsTimerImpl::SetDelay, which then caused the reentrancy. We no longer call SetDelay in DoAfterSleep; we simply do some arithmetic on the timeouts, kick the monitor, and return. I bet this fix would have fixed your deadlock too.
Flags: needinfo?(docfaraday)
Reporter | ||
Comment 59•9 years ago
|
||
@mcmanus Hmm, looking at the stack in https://crash-stats.mozilla.org/report/index/5dbc5816-6b99-409f-9286-089bb2150704, it doesn't make any sense. The stack has nsTimerImpl::SetDelay calling nsIOService::Observe, which is definitely not something that the code has ever done. That stack is trashed, I think. I don't think this bug could have caused stack corruption, though.
Comment 60•9 years ago
|
||
The stack is a little weird, but I think the bigger point about it is how PostSleepWakeNotification calls all of its observers (including the timer code) re-rentrantly from whatever happened to be interrupted when you went to sleep. In the case of the stack trace in that crash it is network code (send()), but it seems like it could just as easily be timer code where send() sits in that trace.
Comment 61•9 years ago
|
||
if this turns out to be more theoretical than practical then just fixing https://bugzilla.mozilla.org/show_bug.cgi?id=1186074 on nightly only makes sense.
Updated•9 years ago
|
Group: core-security → core-security-release
Comment 62•9 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5:
fixed → ---
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•