Bug 198496 - NetworkHTTPSUpgradeChecker should construct and destruct database on the background thread
Summary: NetworkHTTPSUpgradeChecker should construct and destruct database on the back...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-03 14:02 PDT by Sihui Liu
Modified: 2019-06-06 12:16 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2019-06-03 14:07 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (4.35 KB, patch)
2019-06-04 10:52 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (4.35 KB, patch)
2019-06-04 13:16 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (4.29 KB, patch)
2019-06-06 11:33 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-06-03 14:02:44 PDT
We are seeing crashes:
Thread 1 name:  Dispatch queue: HTTPS Upgrade Checker Thread
Thread 1 Crashed ↩:
0   libsqlite3.dylib              	0x1ba202ac sqlite3MutexMisuseAssert + 136 
1   libsqlite3.dylib              	0x1ba202a0 sqlite3MutexMisuseAssert + 124
2   libsqlite3.dylib              	0x1ba200dc checkMutexEnter + 56 
3   libsqlite3.dylib              	0x1b97469c sqlite3_overload_function + 140 
4   libsqlite3.dylib              	0x1b96e288 openDatabase + 3288 
5   WebCore                       	0x23f8d69c WebCore::SQLiteDatabase::open(WTF::String const&, WebCore::SQLiteDatabase::OpenMode) + 344 (SQLiteDatabase.cpp:116)
6   WebKit                        	0x02ebae70 WTF::Detail::CallableWrapper<WebKit::NetworkHTTPSUpgradeChecker::NetworkHTTPSUpgradeChecker()::$_18, void>::call() + 80 (NetworkHTTPSUpgradeChecker.cpp:70)
7   libdispatch.dylib             	0x1b02d7c0 _dispatch_call_block_and_release + 24 (init.c:1408)
8   libdispatch.dylib             	0x1b02eb50 _dispatch_client_callout + 16 (object.m:495)
Comment 1 Sihui Liu 2019-06-03 14:03:13 PDT
<rdar://problem/50795714>
Comment 2 Sihui Liu 2019-06-03 14:07:36 PDT
Created attachment 371210 [details]
Patch
Comment 3 Chris Dumez 2019-06-03 14:09:24 PDT
Comment on attachment 371210 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +

Can you please explain the "fix" in the change log?
Comment 4 Chris Dumez 2019-06-03 14:10:27 PDT
Comment on attachment 371210 [details]
Patch

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

> Source/WebKit/ChangeLog:5
> +        <rdar://problem/50795714>

Should this be a sub-radar of this one? What about all the IDB crashes?
Comment 5 Chris Dumez 2019-06-03 14:15:03 PDT
Comment on attachment 371210 [details]
Patch

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

Are we sure the issue is not that we're using a background queue as opposed to a background *thread*? Meaning that we do use multiple threads but only 1 thread at a time given that the queue is serial?

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:-58
> -    , m_database(makeUniqueRef<WebCore::SQLiteDatabase>())

I do not understand why we need to construct this on the background queue. The SQLiteDatabase constructor does not call any of the sqlite API as far as I know.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:-59
> -    , m_statement(makeUniqueRef<WebCore::SQLiteStatement>(m_database.get(), "SELECT host FROM hosts WHERE host = ?;"_s))

ditto.
Comment 6 Chris Dumez 2019-06-03 14:30:02 PDT
Comment on attachment 371210 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:70
> +        m_database = std::make_unique<SQLiteDatabase>();

Also, now these get constructed on the background queue but still destroyed on the main thread, which may not be safe.
Comment 7 Chris Dumez 2019-06-03 15:36:37 PDT
Comment on attachment 371210 [details]
Patch

Note that I am not opposed to making such a change, especially if this is how our usual database code does it. However:
1. I do not understand why this would fix the crash in question
2. These objects should get destroyed on the background queue if they get constructed on the background queue to avoid introducing new thread-safety bugs
Comment 8 Sihui Liu 2019-06-04 10:52:15 PDT
(In reply to Chris Dumez from comment #7)
> Comment on attachment 371210 [details]
> Patch
> 
> Note that I am not opposed to making such a change, especially if this is
> how our usual database code does it. However:
> 1. I do not understand why this would fix the crash in question
> 2. These objects should get destroyed on the background queue if they get
> constructed on the background queue to avoid introducing new thread-safety
> bugs

1. The bug should already be fixed as discussed in email. I will change the changelog.
2. Correct. I saw the comment "// This object should be owned by a singleton object." so I didn't destroy the objects. NetworkProcess is not a singleton now, we should add this.
Comment 9 Sihui Liu 2019-06-04 10:52:39 PDT
Created attachment 371300 [details]
Patch
Comment 10 Chris Dumez 2019-06-04 10:56:22 PDT
Comment on attachment 371300 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:42
> +constexpr auto HTTPSUpgradeCheckerQuery = "SELECT host FROM hosts WHERE host = ?"_s;

nit: httpsUpgradeCheckerQuery per WebKit coding style.
Comment 11 Sihui Liu 2019-06-04 13:16:26 PDT
Created attachment 371326 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2019-06-04 13:47:33 PDT
Comment on attachment 371326 [details]
Patch for landing

Rejecting attachment 371326 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
CURSOR_SCALE -DENABLE_NETWORK_CACHE_SPECULATIVE_REVALIDATION -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_EVENTS -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESIZE_OBSERVER -DENABLE_RESOURCE_LOAD_STATISTICS -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SANDBOX_EXTENSIONS -DENABLE_SERVER_PRECONNECT -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SHAREABLE_RESOURCE -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USERSELECT_ALL -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_VARIATION_FONTS -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEBDRIVER_MOUSE_INTERACTIONS -DENABLE_WEBDRIVER_KEYBOARD_INTERACTIONS -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_CRYPTO -DENABLE_WEB_PROCESS_SANDBOX -DENABLE_WEB_RTC -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DENABLE_MANUAL_SANDBOXING -DHAVE_CORE_PREDICTION -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -DFRAMEWORK_NAME=WebKit -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -fasm-blocks -fstrict-aliasing -Wprotocol -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.13 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -Wcomma -Wblock-capture-autoreleasing -Wstrict-prototypes -Wrange-loop-analysis -Wno-semicolon-before-method-body -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/PrivateHeaders/ForwardingHeaders -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2 -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/WebKitAdditions -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/local/include/WebKitAdditions -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/local/include/webrtc -I/Volumes/Data/EWS/WebKit/Source/WebKit -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat-security -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wno-unused-parameter -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebKit2Prefix-gljelmzeecuvvkhaymcxhnhphybs/WebKit2Prefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource20-mm.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource20-mm.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource20-mm.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource20-mm.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/UnifiedSource3.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2/unified-sources/UnifiedSource3.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: https://webkit-queues.webkit.org/results/12376773
Comment 13 Sihui Liu 2019-06-06 11:33:31 PDT
Created attachment 371513 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2019-06-06 12:16:03 PDT
Comment on attachment 371513 [details]
Patch for landing

Clearing flags on attachment: 371513

Committed r246163: <https://trac.webkit.org/changeset/246163>
Comment 15 WebKit Commit Bot 2019-06-06 12:16:04 PDT
All reviewed patches have been landed.  Closing bug.