Bug 198911

Created attachment 372236 [details] Patch

Created attachment 372237 [details] Patch

Created attachment 372261 [details] Patch

Created attachment 372303 [details] Patch

Created attachment 372319 [details] Patch

Created attachment 372329 [details] Patch

Created attachment 372331 [details] Patch

Can you provide some details about the use case that requires this behavior? The reason I ask is that -- as you pointed out -- it's a bit of a layering violation for WebCore to require JavaScriptCore to lock WebCore, and also for JavaScriptCore to require WebCore to auto-unlock itself under certain runloop conditions. Also, we'd like to reduce the impact of the WebThread design in our code, rather than increasing it. Access to JSContext is not API in UIWebView. For SPI clients, the expectation is that they should perform JSContext operations either on the WebThread or while already holding the WebThread lock. If a client has neglected to do that, it might be better just to fix the client. Clients that call into WebCore's JSContext usually call into WebKit's APIs as well, and if they're doing this without holding the WebThreadLock, they might have other problems.

(In reply to Geoffrey Garen from comment #9) > Can you provide some details about the use case that requires this behavior? The original report is rdar://42258767. Multiple third party applications are touching JSContext of UIWebView. My guess is they are obtaining JSContext by executing `[uiWebView valueForKeyPath:@"documentView.webView.mainFrame.javaScriptContext"]`. They interact with the JSContext directly[1], and the JSContext does not take the WebThread lock. Eventually, GC happens, and a part of incremental sweeper task is executed when interacting with JSContext from the main thread. So incremental sweeper's task is executed in the main thread instead of the WebThread without taking the WebThread lock. And the incremental sweeper destroys WebCore::Timer, and WebCore::TimerBase's assertion hits. The assertion was originally introduced in https://trac.webkit.org/changeset/233821/webkit. RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get())); To mitigate that we changed the above assertion to the following in https://trac.webkit.org/changeset/234778/webkit RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()) || shouldSuppressThreadSafetyCheck()); The above is just suppressing the assertion. Even though the GC does not happen, interactions through JSContext of UIWebView are potentially dangerous because it can modify WebCore things without taking the WebThread lock. For example, if js code calls WebCore DOM function, and if the DOM function modifies some part of WebCore, and if WebThread is simultaneously modifying the same thing, this can lead to memory corruption. As described in the bug of the assertion change (https://bugs.webkit.org/show_bug.cgi?id=188480#c6), the above change is changing the assertion hit to the random memory corruption. [1]: The original crash log indicates that it is touching [JSValue valueForProperty]. > The reason I ask is that -- as you pointed out -- it's a bit of a layering > violation for WebCore to require JavaScriptCore to lock WebCore, and also > for JavaScriptCore to require WebCore to auto-unlock itself under certain > runloop conditions. Also, we'd like to reduce the impact of the WebThread > design in our code, rather than increasing it. Agreed. > Access to JSContext is not API in UIWebView. For SPI clients, the > expectation is that they should perform JSContext operations either on the > WebThread or while already holding the WebThread lock. If a client has > neglected to do that, it might be better just to fix the client. Right. The problem is that this is caused by third party applications. Maybe, unfortunately, this is commonly used... I searched "JSContext UIWebView" in Google, and it shows https://stackoverflow.com/questions/21714365/uiwebview-javascript-losing-reference-to-ios-jscontext-namespace-object immediately. And if we search "documentView.webView.mainFrame.javaScriptContext" in GitHub, we see so many codes using this. https://github.com/search?q=documentView.webView.mainFrame.javaScriptContext&type=Code The original rdar://42258767 describes the third party applications causing this crash. To avoid random memory corruption, I think we have two ways to fix this issue. (1) Make JSC.framework aware of WebThread (this patch) or (2) crash immediately when JSLock of UIWebView commonVM is held without WebThread lock. Currently, I took (1)'s way. > Clients that call into WebCore's JSContext usually call into WebKit's APIs > as well, and if they're doing this without holding the WebThreadLock, they > might have other problems. Yes, this problem only happens when the clients directly interact with JSContext of UIWebView.

(In reply to Yusuke Suzuki from comment #10) > (In reply to Geoffrey Garen from comment #9) > > Can you provide some details about the use case that requires this behavior? > > The original report is rdar://42258767. Multiple third party applications > are touching JSContext of UIWebView. > My guess is they are obtaining JSContext by executing `[uiWebView > valueForKeyPath:@"documentView.webView.mainFrame.javaScriptContext"]`. > They interact with the JSContext directly[1], and the JSContext does not > take the WebThread lock. > Eventually, GC happens, and a part of incremental sweeper task is executed > when interacting with JSContext from the main thread. > So incremental sweeper's task is executed in the main thread instead of the > WebThread without taking the WebThread lock. > And the incremental sweeper destroys WebCore::Timer, and > WebCore::TimerBase's assertion hits. > The assertion was originally introduced in > https://trac.webkit.org/changeset/233821/webkit. > > RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get())); > > To mitigate that we changed the above assertion to the following in > https://trac.webkit.org/changeset/234778/webkit > > RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()) || > shouldSuppressThreadSafetyCheck()); > > The above is just suppressing the assertion. > Even though the GC does not happen, interactions through JSContext of > UIWebView are potentially dangerous because it can modify WebCore things > without taking the WebThread lock. > For example, if js code calls WebCore DOM function, and if the DOM function > modifies some part of WebCore, and if WebThread is simultaneously modifying > the same thing, this can lead to memory corruption. > As described in the bug of the assertion change > (https://bugs.webkit.org/show_bug.cgi?id=188480#c6), the above change is > changing the assertion hit to the random memory corruption. > > [1]: The original crash log indicates that it is touching [JSValue > valueForProperty]. Ignore the "incremental sweeper" word. UIWebView is disabling JIT. So JS VM is mini mode VM. In this configuration, incremental sweeper is disabled, and instead we sweep objects synchronously, as a result, sweeping happens in the main thread. The other part of the above clarification is the same. The sweep happens in the main thread instead of the WebThread without taking the WebThread lock, and the above assertion hits.

Note that whether the incremental sweeper is enabled is not directly related to the crash issue in this bug :)

@Geoff, I also added more detailed explanation in the radar (because it involves private crash trace data). Does the above clarification make sense?

Comment on attachment 372331 [details] Patch Thanks for the clarification. Sad face. I tried for a little while to come up with a better solution, but I don't have one. So, I guess I'll say r+.

Thank you for your review. (In reply to Geoffrey Garen from comment #14) > Comment on attachment 372331 [details] > Patch > > Thanks for the clarification. > > Sad face. Yeah, I really want to drop WebKitLegacy and stop considering about WebThread. And it is sad to me too that many clients are using JSContext of UIWebView...... > > I tried for a little while to come up with a better solution, but I don't > have one. So, I guess I'll say r+.

Committed r246578: <https://trac.webkit.org/changeset/246578>