Bug 198496 – NetworkHTTPSUpgradeChecker should construct and destruct database on the background thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

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)

[Sihui Liu]

<rdar://problem/50795714>

[Sihui Liu]

Created attachment 371210 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

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?

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 371300 [details]
Patch

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 371326 [details]
Patch for landing

[WebKit Commit Bot]

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

[Sihui Liu]

Created attachment 371513 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 371513 [details]
Patch for landing

Clearing flags on attachment: 371513

Committed r246163: <https://trac.webkit.org/changeset/246163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.