Summary: | REGRESSION (r268384): ASSERTION FAILED: _startCount > 1 in -[WKMouseDeviceObserver stop] | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||
Component: | New Bugs | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | hi, thorton, webkit-bot-watchers-bugzilla, webkit-bug-importer, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=212580 | ||||||||||
Attachments: |
|
Description
Ryan Haddad
2020-10-13 17:03:37 PDT
Created attachment 411283 [details]
Patch
I'm not sure why/how this is happening, but the `ASSERT` isn't really necessary
Comment on attachment 411283 [details]
Patch
You need to understand how you got here; a double-stop could mean you go to zero while having outstanding clients, so r- until you figure out how.
(In reply to Tim Horton from comment #3) > Comment on attachment 411283 [details] > Patch > > You need to understand how you got here; a double-stop could mean you go to zero while having outstanding clients, so r- until you figure out how. here's what I know so far: The log above indicates that these tests are failing: - WKWebView.EvaluateJavaScriptBlockCrash - WebProcessCache.ClearWhenEnteringCache - WKProcessPool.WarmInitialProcess - WKProcessPool.PrewarmedProcessCrash - WKWebView.LocalStorageClear - ProcessSwap.ClosePageAfterCrossSiteProvisionalLoad - WKNavigation.ProcessCrashDuringCallback - _WKDownload.CrashAfterDownloadDidFinishWhenDownloadProxyHoldsTheLastRefOnWebProcessPool None of these tests crash on my machine using an iOS simulator. There is only one place that `[WKMouseDeviceObserver stop]` is called, and that's inside `WebProcessProxy::platformDestroy` (private member function), which is called inside `WebProcessProxy::~WebProcessProxy`. `[WKMouseDeviceObserver start]` is called inside `WebProcessProxy::platformCreate` (private member function), which is called inside `WebProcessProxy::WebProcessProxy`, and inside `Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSMouseSupport.mm` (all of which ran before any test crashed). I know that `WebProcessProxy::WebProcessProxy` and `WebProcessProxy::~WebProcessProxy` are not somehow being called more than once because there are other `ASSERT` in both that should trigger in that case. Even more confusingly, `WKStylusDeviceObserver` has the exact same logic in the exact same places as `WKMouseDeviceObserver` and yet its `ASSERT` is not being triggered. clearly there is something magical happening here :( I will try to reproduce this tonight by running the entire API test suite, but I wonder if it wouldn't be more prudent to just change `WKMouseDeviceObserver`/`WKStylusDeviceObserver` to add the listener as soon as the `sharedInstance` is created and remove `stop` (I only added it to try to be more efficient when there are no `WebProcessProxy`, but I think that that's likely very rare) instead of trying to chase down the cause of what should be impossible. I'll see what I find tonight. Created attachment 411294 [details] Patch (In reply to Devin Rousso from comment #4) > clearly there is something magical happening here :( I have found the magic! It was past me being stupid 😬 It should be `_startCount >= 1` as it's totally possible for there to have been only one prior call to `-[WKMouseDeviceObserver start]`/`-[WKStylusDeviceObserver start]`. I tested this change locally and it passed all the API tests with no crashes. I think I'd still suggest the patch as I've written it as it's more permissive/forgiving (e.g. if someone _really_ wants to make sure the listener has stopped then they can call it without having to worry about a crash). If there is a preference/precedence for ensuring that things like this listener are not over-stopped then I can just adjust the `ASSERT`. (In reply to Devin Rousso from comment #5) > Created attachment 411294 [details] > Patch > > (In reply to Devin Rousso from comment #4) > > clearly there is something magical happening here :( > > I have found the magic! It was past me being stupid 😬 > > It should be `_startCount >= 1` as it's totally possible for there to have > been only one prior call to `-[WKMouseDeviceObserver > start]`/`-[WKStylusDeviceObserver start]`. > > I tested this change locally and it passed all the API tests with no crashes. > > I think I'd still suggest the patch as I've written it as it's more > permissive/forgiving (e.g. if someone _really_ wants to make sure the > listener has stopped then they can call it without having to worry about a > crash). This is very strange reasoning for exactly the reason I said the first time around (your "someone" who "really wants to make sure the listener has stopped" is screwing over someone /else/ who is using your global API). Counted APIs need to be /very strict/. > If there is a preference/precedence for ensuring that things like > this listener are not over-stopped then I can just adjust the `ASSERT`. Plz put the assert back. Created attachment 411351 [details]
Patch
Committed r268482: <https://trac.webkit.org/changeset/268482> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411351 [details]. |