Bug 188426 – JSRunLoopTimer may run part of a member function after it's destroyed

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Let's consider this function:

```
void JSRunLoopTimer::timerDidFire()
{
    JSLock* apiLock = m_apiLock.get();
    if (!apiLock) {
        // Likely a buggy usage: the timer fired while JSRunLoopTimer was being destroyed.
        return;
    }
    // HERE
    std::lock_guard<JSLock> lock(*apiLock);
    RefPtr<VM> vm = apiLock->vm();
    if (!vm) {
        // The VM has been destroyed, so we should just give up.
        return;
    }

    doWork();
}
```

Look at the comment "HERE" above. Let's say the thread gets context switched out for X seconds at that point. And let's say that during that X seconds, the VM is destroyed. Transitively, the VM owns this JSRunLoopTimer (e.g, GCActivityCallback). If that happens, we're now accessing fields on a dead object.

(An easy way to reproduce this bug is to just drop a sleep at the "HERE" comment).

[Saam Barati]

After thinking about this, it seems like the only reasonable way to ensure the lifetime is for this class to ref() itself when it sets a timer, and deref when the callback is called. But this get's slightly tricky with how we use these RunLoopTimer objects.

[Saam Barati]

Created attachment 346884 [details]
patch

[EWS Watchlist]

Comment on attachment 346884 [details]
patch

Attachment 346884 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8814816

New failing tests:
imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-mutationrecord-001.html

[EWS Watchlist]

Created attachment 346885 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346884 [details]
patch

Attachment 346884 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8815017

New failing tests:
editing/selection/select-bidi-run.html

[EWS Watchlist]

Created attachment 346887 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346884 [details]
patch

Attachment 346884 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8815036

New failing tests:
fast/events/fire-scroll-event-element.html

[EWS Watchlist]

Created attachment 346888 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Saam Barati]

I need to handle the VM dying before the timer.

[Saam Barati]

Created attachment 346890 [details]
patch

[EWS Watchlist]

Comment on attachment 346890 [details]
patch

Attachment 346890 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8816225

New failing tests:
workers/bomb.html

[EWS Watchlist]

Created attachment 346891 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Saam Barati]

Created attachment 346892 [details]
patch

[Saam Barati]

Created attachment 346893 [details]
patch

[Saam Barati]

Created attachment 346894 [details]
patch

[Mark Lam]

Comment on attachment 346894 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346894&action=review

Some comments for now.  Per our offline discussion, I'll wait for the next patch with bug fixes before continuing the review.

> Source/JavaScriptCore/heap/GCActivityCallback.cpp:60
>      doCollection();

I think you should update the year on the copyright header.

> Source/JavaScriptCore/heap/GCActivityCallback.cpp:69
>      if (newDelay * timerSlop > m_delay)
>          return;
>      Seconds delta = m_delay - newDelay;
>      m_delay = newDelay;
> -    CFRunLoopTimerSetNextFireDate(m_timer.get(), CFRunLoopTimerGetNextFireDate(m_timer.get()) - delta.seconds());
> +    setTimeUntilFire(timeUntilFire() - delta);

This logic is now broken because you no longer reset m_delay to s_decade in cancelTimer().  You still need cancelTimer() just for the purpose of resetting m_delay.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:211
> +    if (m_isTimerFiringForBalancedDeref) {

Use UNLIKELY() for this condition?

[Saam Barati]

Created attachment 346929 [details]
patch

[Saam Barati]

Comment on attachment 346929 [details]
patch

I have an easier way to do this.

[Saam Barati]

Created attachment 346996 [details]
WIP

[EWS Watchlist]

Attachment 346996 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:89:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:198:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 346996 [details]
WIP

Attachment 346996 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8841409

New failing tests:
http/wpt/resource-timing/rt-initiatorType.worker.html

[EWS Watchlist]

Created attachment 346997 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346996 [details]
WIP

Attachment 346996 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8841494

New failing tests:
http/wpt/resource-timing/rt-initiatorType.worker.html

[EWS Watchlist]

Created attachment 347000 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346996 [details]
WIP

Attachment 346996 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8841492

New failing tests:
http/wpt/resource-timing/rt-initiatorType.worker.html

[EWS Watchlist]

Created attachment 347001 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 346996 [details]
WIP

Attachment 346996 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8842144

New failing tests:
http/wpt/resource-timing/rt-initiatorType.worker.html

[EWS Watchlist]

Created attachment 347005 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Saam Barati]

Created attachment 347021 [details]
WIP

[EWS Watchlist]

Attachment 347021 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:94:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:203:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 347021 [details]
WIP

Attachment 347021 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8846401

New failing tests:
http/wpt/resource-timing/rt-initiatorType.worker.html

[EWS Watchlist]

Created attachment 347029 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Saam Barati]

Created attachment 347030 [details]
WIP

[EWS Watchlist]

Attachment 347030 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:136:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:241:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 347031 [details]
WIP

[EWS Watchlist]

Attachment 347031 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:136:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:241:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 347032 [details]
WIP

[EWS Watchlist]

Attachment 347032 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:240:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 347056 [details]
WIP

[Saam Barati]

Created attachment 347065 [details]
patch

[EWS Watchlist]

Attachment 347065 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:233:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 347065 [details]
patch

Can't a JSRunLoopTimer just ref() itself inside JSRunLoopTimer::scheduleTimer() and deref() itself inside JSRunLoopTimer::cancelTimer()?

[Keith Miller]

(In reply to Geoffrey Garen from comment #42)
> Comment on attachment 347065 [details]
> patch
> 
> Can't a JSRunLoopTimer just ref() itself inside
> JSRunLoopTimer::scheduleTimer() and deref() itself inside
> JSRunLoopTimer::cancelTimer()?

No, if another thread calls cancelTimer() after CFRunLoop has called our callback but before the JSRunLoopTimers has managed to ref itself. You'd now be running a function with a freed this.

[Geoffrey Garen]

(In reply to Keith Miller from comment #43)
> (In reply to Geoffrey Garen from comment #42)
> > Comment on attachment 347065 [details]
> > patch
> > 
> > Can't a JSRunLoopTimer just ref() itself inside
> > JSRunLoopTimer::scheduleTimer() and deref() itself inside
> > JSRunLoopTimer::cancelTimer()?
> 
> No, if another thread calls cancelTimer() after CFRunLoop has called our
> callback but before the JSRunLoopTimers has managed to ref itself. You'd now
> be running a function with a freed this.

If you ref() inside JSRunLoopTimer::scheduleTimer(), you already hold a ref when CFRunLoop calls your callback.

[Mark Lam]

(In reply to Geoffrey Garen from comment #44)
> (In reply to Keith Miller from comment #43)
> > (In reply to Geoffrey Garen from comment #42)
> > > Comment on attachment 347065 [details]
> > > patch
> > > 
> > > Can't a JSRunLoopTimer just ref() itself inside
> > > JSRunLoopTimer::scheduleTimer() and deref() itself inside
> > > JSRunLoopTimer::cancelTimer()?
> > 
> > No, if another thread calls cancelTimer() after CFRunLoop has called our
> > callback but before the JSRunLoopTimers has managed to ref itself. You'd now
> > be running a function with a freed this.
> 
> If you ref() inside JSRunLoopTimer::scheduleTimer(), you already hold a ref
> when CFRunLoop calls your callback.

I believe the issue is that the run loop's timer callback may have started but got pre-empted before it can ref the Timer object.  Meanwhile on the main thread, the VM shutdowns and frees the Timer object.  Eventually, the timer thread gets to run and ref's the now freed Timer object and reads its fields ... which is what we're trying to prevent.

[Mark Lam]

Comment on attachment 347065 [details]
patch

The mac-debug EWS build failures looks legit.

[Mark Lam]

(In reply to Mark Lam from comment #45)
> (In reply to Geoffrey Garen from comment #44)
> > (In reply to Keith Miller from comment #43)
> > > (In reply to Geoffrey Garen from comment #42)
> > > > Comment on attachment 347065 [details]
> > > > patch
> > > > 
> > > > Can't a JSRunLoopTimer just ref() itself inside
> > > > JSRunLoopTimer::scheduleTimer() and deref() itself inside
> > > > JSRunLoopTimer::cancelTimer()?
> > > 
> > > No, if another thread calls cancelTimer() after CFRunLoop has called our
> > > callback but before the JSRunLoopTimers has managed to ref itself. You'd now
> > > be running a function with a freed this.
> > 
> > If you ref() inside JSRunLoopTimer::scheduleTimer(), you already hold a ref
> > when CFRunLoop calls your callback.
> 
> I believe the issue is that the run loop's timer callback may have started
> but got pre-empted before it can ref the Timer object.  Meanwhile on the
> main thread, the VM shutdowns and frees the Timer object.  Eventually, the
> timer thread gets to run and ref's the now freed Timer object and reads its
> fields ... which is what we're trying to prevent.

Nevermind.  You said "inside JSRunLoopTimer::scheduleTimer()" as in asymmetric ref/deref.  I believe Saam identified some complications with cancelTimer() when doing that, but we'll take a second look.

[Saam Barati]

(In reply to Geoffrey Garen from comment #44)
> (In reply to Keith Miller from comment #43)
> > (In reply to Geoffrey Garen from comment #42)
> > > Comment on attachment 347065 [details]
> > > patch
> > > 
> > > Can't a JSRunLoopTimer just ref() itself inside
> > > JSRunLoopTimer::scheduleTimer() and deref() itself inside
> > > JSRunLoopTimer::cancelTimer()?
> > 
> > No, if another thread calls cancelTimer() after CFRunLoop has called our
> > callback but before the JSRunLoopTimers has managed to ref itself. You'd now
> > be running a function with a freed this.
> 
> If you ref() inside JSRunLoopTimer::scheduleTimer(), you already hold a ref
> when CFRunLoop calls your callback.

Yes, this would work. But the tricky thing is getting cancel correct. The goal being: we want to be able to deref() in cancel so we don't leak.

If you want to see what I came up with to solve this, the patch for that is here:
https://bugs.webkit.org/attachment.cgi?id=346929&action=review

I think that approach turned out to be a lot more complicated than what I have up for review now.

The main issue I ran into with the prior approach is this:
1. I schedule a timer. I ref() b/c of this.
2. I cancel a timer. I'd like to deref() here. However, how do I know I've done that in a way that's safe. E.g, have I cancelled before the timer fired? This turns out to be trickier than I expected.

[Saam Barati]

Created attachment 347102 [details]
patch

Fix debug build.

[EWS Watchlist]

Attachment 347102 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:233:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

> 2. I cancel a timer. I'd like to deref() here. However, how do I know I've
> done that in a way that's safe. E.g, have I cancelled before the timer
> fired?

Under what conditions do you cancel a timer, and then it fires afterward anyway?

[EWS Watchlist]

Comment on attachment 347102 [details]
patch

Attachment 347102 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8858423

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 347105 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Saam Barati]

(In reply to Geoffrey Garen from comment #51)
> > 2. I cancel a timer. I'd like to deref() here. However, how do I know I've
> > done that in a way that's safe. E.g, have I cancelled before the timer
> > fired?
> 
> Under what conditions do you cancel a timer, and then it fires afterward
> anyway?

When it's imminently about to fire. For example, let's say we have threads T1 and T2. T1 is running the runloop we're firing on, and T2 is the thread that will cancel the timer.

T1. Timer fires
T1. CF issues a call to our callback.
T1. Before an any callback code is run, T1 gets context switched
T2. Cancels

In such a scenario, T2 and T1 can't *both* deref.

[Geoffrey Garen]

(In reply to Saam Barati from comment #54)
> (In reply to Geoffrey Garen from comment #51)
> > > 2. I cancel a timer. I'd like to deref() here. However, how do I know I've
> > > done that in a way that's safe. E.g, have I cancelled before the timer
> > > fired?
> > 
> > Under what conditions do you cancel a timer, and then it fires afterward
> > anyway?
> 
> When it's imminently about to fire. For example, let's say we have threads
> T1 and T2. T1 is running the runloop we're firing on, and T2 is the thread
> that will cancel the timer.
> 
> T1. Timer fires
> T1. CF issues a call to our callback.
> T1. Before an any callback code is run, T1 gets context switched
> T2. Cancels
> 
> In such a scenario, T2 and T1 can't *both* deref.

Is it a requirement that you should be able to call JSRunLoopTimer::cancelTimer() from any thread at any time? Who does this?

[Saam Barati]

(In reply to Geoffrey Garen from comment #55)
> (In reply to Saam Barati from comment #54)
> > (In reply to Geoffrey Garen from comment #51)
> > > > 2. I cancel a timer. I'd like to deref() here. However, how do I know I've
> > > > done that in a way that's safe. E.g, have I cancelled before the timer
> > > > fired?
> > > 
> > > Under what conditions do you cancel a timer, and then it fires afterward
> > > anyway?
> > 
> > When it's imminently about to fire. For example, let's say we have threads
> > T1 and T2. T1 is running the runloop we're firing on, and T2 is the thread
> > that will cancel the timer.
> > 
> > T1. Timer fires
> > T1. CF issues a call to our callback.
> > T1. Before an any callback code is run, T1 gets context switched
> > T2. Cancels
> > 
> > In such a scenario, T2 and T1 can't *both* deref.
> 
> Is it a requirement that you should be able to call
> JSRunLoopTimer::cancelTimer() from any thread at any time? Who does this?

API users could cause this to happen in arbitrary ways:
- We grab the runloop at VM construction.
- We then can run code on that VM from any other thread. That code can do many things that will cancelTimer()

[Saam Barati]

Looks like HashMap<Ref<P>, SomeVaueWhereEmptyValueIsNotZero> is broken...

[EWS Watchlist]

Comment on attachment 347102 [details]
patch

Attachment 347102 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8873312

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 347232 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Mark Lam]

Comment on attachment 347102 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347102&action=review

r=me.  I agree that this patch is much easier to reason about in terms of synchronization because it's now reduced to sync'ing on the Manager's lock, and the Manager is immortal.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:194
> +    EpochTime fireEpochTime = epochTime(delay);

At first, I was concerned that delay may be negative, and that we'll have some underflow issue with fireEpochTime.  However, I see that EpochTime is Seconds, and therefore uses a double as storage.  I still think that it's strange to have a negative epoch time but I suppose the math can't be messed up because we're doing doubles math.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:208
> +        if (entry.first.ptr() == &timer) {
> +            entry.second = fireEpochTime;
> +            found = true;
> +        }
> +        scheduleTime = std::min(scheduleTime, entry.second);

Shouldn't we break out of the loop if we've found the timer?  Each timer may only be enqueued in data.timers once, right?

Shouldn't the setting of "scheduleTime = std::min(scheduleTime, entry.second);" only be done if the timer is found?  Oh, I see.   You want to find the smallest time of all the JSRunLoopTimers.  Not sure if it's worth it but if you just cache this smallest time value in a PerVMData field, then you can bail out as soon as you've found the matching timer. ... On second thought, probably not worth it.  This is just an optimization which we can apply later if needed.  For now, symmetry with the cancelTimer() case is probably more important to ensure correctness.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:325
> +        Manager::shared().scheduleTimer(*this, intervalInSeconds);

nit: pass the locker to document the contract that we must first lock the JSRunLoopTimer?

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:337
> +    Manager::shared().cancelTimer(*this);

nit: pass the locker to document the contract that we must first lock the JSRunLoopTimer?

> Source/JavaScriptCore/runtime/JSRunLoopTimer.h:64
> +        void willDestroyVM(VM&);

nit: name this unregisterVM for symmetry?

[Saam Barati]

Comment on attachment 347102 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347102&action=review

>> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:325
>> +        Manager::shared().scheduleTimer(*this, intervalInSeconds);
> 
> nit: pass the locker to document the contract that we must first lock the JSRunLoopTimer?

That feels weird since we'd be passing in the locker for a JSRunLoopTimer and not the Manager. Other things could use Manager and not lock around calls to schedule/cancel

>> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:337
>> +    Manager::shared().cancelTimer(*this);
> 
> nit: pass the locker to document the contract that we must first lock the JSRunLoopTimer?

ditto

>> Source/JavaScriptCore/runtime/JSRunLoopTimer.h:64
>> +        void willDestroyVM(VM&);
> 
> nit: name this unregisterVM for symmetry?

sounds good.

[Mark Lam]

(In reply to Saam Barati from comment #61)
> Comment on attachment 347102 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347102&action=review
> 
> >> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:325
> >> +        Manager::shared().scheduleTimer(*this, intervalInSeconds);
> > 
> > nit: pass the locker to document the contract that we must first lock the JSRunLoopTimer?
> 
> That feels weird since we'd be passing in the locker for a JSRunLoopTimer
> and not the Manager. Other things could use Manager and not lock around
> calls to schedule/cancel

OK.

[Saam Barati]

Created attachment 347584 [details]
patch for landing

[EWS Watchlist]

Attachment 347584 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:233:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 347591 [details]
patch for landing

[EWS Watchlist]

Attachment 347591 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:235:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 347591 [details]
patch for landing

Clearing flags on attachment: 347591

Committed r235107: <https://trac.webkit.org/changeset/235107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like https://trac.webkit.org/changeset/235107/webkit

Has caused this crash to occur in test:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fset-protocol.html

Crash:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r235107%20(5999)/http/tests/websocket/tests/hybi/set-protocol-crash-log.txt

Excerpt:
stderr:
ASSERTION FAILED: mapIterator->value.contains(url)
/Volumes/Data/slave/ios-simulator-11-debug/build/Source/WebKit/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp(125) : void WebKit::NetworkBlobRegistry::unregisterBlobURL(WebKit::NetworkConnectionToWebProcess *, const WebCore::URL &)
1   0x11ccd3be9 WTFCrash
2   0x10e72271b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x10ec24200 WebKit::NetworkBlobRegistry::unregisterBlobURL(WebKit::NetworkConnectionToWebProcess*, WebCore::URL const&)
4   0x10eb49aed WebKit::NetworkConnectionToWebProcess::unregisterBlobURL(WebCore::URL const&)

Looks like one of the assertions added in this patch is causing the crash.

[Saam Barati]

(In reply to Truitt Savell from comment #69)
> It looks like https://trac.webkit.org/changeset/235107/webkit
> 
> Has caused this crash to occur in test:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fset-
> protocol.html
> 
> Crash:
> https://build.webkit.org/results/
> Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r235107%20(5999)/http/
> tests/websocket/tests/hybi/set-protocol-crash-log.txt
> 
> Excerpt:
> stderr:
> ASSERTION FAILED: mapIterator->value.contains(url)
> /Volumes/Data/slave/ios-simulator-11-debug/build/Source/WebKit/
> NetworkProcess/FileAPI/NetworkBlobRegistry.cpp(125) : void
> WebKit::NetworkBlobRegistry::unregisterBlobURL(WebKit::
> NetworkConnectionToWebProcess *, const WebCore::URL &)
> 1   0x11ccd3be9 WTFCrash
> 2   0x10e72271b WTFCrashWithInfo(int, char const*, char const*, int)
> 3   0x10ec24200
> WebKit::NetworkBlobRegistry::unregisterBlobURL(WebKit::
> NetworkConnectionToWebProcess*, WebCore::URL const&)
> 4   0x10eb49aed
> WebKit::NetworkConnectionToWebProcess::unregisterBlobURL(WebCore::URL const&)
> 
> Looks like one of the assertions added in this patch is causing the crash.

This is really weird. Do we even make a VM in the network process?

[Saam Barati]

I'm looking into this now.

[Saam Barati]

I can't reproduce this.

[Ryan Haddad]

I can reproduce the crash with a ToT build with the following:
run-webkit-tests http/tests/websocket/tests/hybi --debug --exit-after-n-crashes-or-timeouts 1 --no-retry -fg --iter 100

I have not yet tried to repro with a build before https://trac.webkit.org/changeset/235107

[Saam Barati]

(In reply to Ryan Haddad from comment #73)
> I can reproduce the crash with a ToT build with the following:
> run-webkit-tests http/tests/websocket/tests/hybi --debug
> --exit-after-n-crashes-or-timeouts 1 --no-retry -fg --iter 100
> 
> I have not yet tried to repro with a build before
> https://trac.webkit.org/changeset/235107

When I do this, every single test fails. Is that your experience as well?

[Saam Barati]

(In reply to Saam Barati from comment #74)
> (In reply to Ryan Haddad from comment #73)
> > I can reproduce the crash with a ToT build with the following:
> > run-webkit-tests http/tests/websocket/tests/hybi --debug
> > --exit-after-n-crashes-or-timeouts 1 --no-retry -fg --iter 100
> > 
> > I have not yet tried to repro with a build before
> > https://trac.webkit.org/changeset/235107
> 
> When I do this, every single test fails. Is that your experience as well?

This is because I had a different apache installed...

[Saam Barati]

(In reply to Ryan Haddad from comment #73)
> I can reproduce the crash with a ToT build with the following:
> run-webkit-tests http/tests/websocket/tests/hybi --debug
> --exit-after-n-crashes-or-timeouts 1 --no-retry -fg --iter 100
> 
> I have not yet tried to repro with a build before
> https://trac.webkit.org/changeset/235107

Did you really run this on the parent directory and not the individual test?

[Saam Barati]

I still can't reproduce this. I need help from y'all on how you're doing it. What OS were you on Ryan?

It would be really helpful if someone can confirm that it can repro at my revision and can't repro on the revision before.

[Saam Barati]

(In reply to Saam Barati from comment #77)
> I still can't reproduce this. I need help from y'all on how you're doing it.
> What OS were you on Ryan?
> 
> It would be really helpful if someone can confirm that it can repro at my
> revision and can't repro on the revision before.

I spoke too soon. I just got it to reproduce.

[Saam Barati]

This appears to be revealing a bug in the blob code.

I've made it so that no JSC timers run, and this crash still happens.

[Saam Barati]

Created attachment 347747 [details]
attaching logs of stack traces

This code seems super innocuous. We're calling a GC from some JS code injected into the test runner.

[Saam Barati]

This patch is also a performance regression :(

[WebKit Commit Bot]

Re-opened since this is blocked by bug 188832

[Saam Barati]

I can’t reproduce the performance regression on my laptop 🤔

I’ll need to see if I can reproduce on other HW

[Saam Barati]

Created attachment 347825 [details]
patch for landing

[Saam Barati]

Comment on attachment 347591 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=347591&action=review

> Source/JavaScriptCore/heap/GCActivityCallback.cpp:72
> +        setTimeUntilFire(delta);

The perf bug is here. This should be:
setTimeUntilFire(newDelay)

[EWS Watchlist]

Attachment 347825 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:235:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 347825 [details]
patch for landing

Clearing flags on attachment: 347825

Committed r235261: <https://trac.webkit.org/changeset/235261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.