Changeset 251362 in webkit
- Timestamp:
- Oct 21, 2019 2:21:14 AM (5 years ago)
- Location:
- trunk/Source/WebKit
- Files:
-
- 3 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebKit/ChangeLog
r251361 r251362 1 2019-10-21 Carlos Garcia Campos <cgarcia@igalia.com> 2 3 [GTK][WPE] IconDatabase is not thread safe yet 4 https://bugs.webkit.org/show_bug.cgi?id=202980 5 6 Reviewed by Adrian Perez de Castro. 7 8 Current implementation is safer, but we still need to protect members used by both threads. 9 10 * UIProcess/API/glib/IconDatabase.cpp: 11 (WebKit::IconDatabase::populatePageURLToIconURLMap): 12 (WebKit::IconDatabase::clearLoadedIconsTimerFired): 13 (WebKit::IconDatabase::checkIconURLAndSetPageURLIfNeeded): 14 (WebKit::IconDatabase::loadIconForPageURL): 15 (WebKit::IconDatabase::iconURLForPageURL): 16 (WebKit::IconDatabase::setIconForPageURL): 17 (WebKit::IconDatabase::clear): 18 * UIProcess/API/glib/IconDatabase.h: 19 1 20 2019-10-21 Tim Horton <timothy_horton@apple.com> 2 21 -
trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp
r250518 r251362 52 52 , m_clearLoadedIconsTimer(RunLoop::main(), this, &IconDatabase::clearLoadedIconsTimerFired) 53 53 { 54 ASSERT(isMainThread()); 54 55 m_clearLoadedIconsTimer.setPriority(RunLoopSourcePriority::ReleaseUnusedResourcesTimer); 55 56 … … 108 109 IconDatabase::~IconDatabase() 109 110 { 111 ASSERT(isMainThread()); 112 110 113 BinarySemaphore semaphore; 111 114 m_workQueue->dispatch([&] { … … 122 125 bool IconDatabase::createTablesIfNeeded() 123 126 { 127 ASSERT(!isMainThread()); 128 124 129 if (m_db.tableExists("IconInfo") && m_db.tableExists("IconData") && m_db.tableExists("PageURL") && m_db.tableExists("IconDatabaseInfo")) 125 130 return false; … … 178 183 void IconDatabase::populatePageURLToIconURLMap() 179 184 { 185 ASSERT(!isMainThread()); 186 180 187 if (!m_db.isOpen()) 181 188 return; … … 189 196 190 197 auto result = query.step(); 198 LockHolder lockHolder(m_pageURLToIconURLMapLock); 191 199 while (result == SQLITE_ROW) { 192 200 m_pageURLToIconURLMap.set(query.getColumnText(0), query.getColumnText(1)); … … 199 207 void IconDatabase::clearStatements() 200 208 { 201 RELEASE_ASSERT(m_db.isOpen()); 209 ASSERT(!isMainThread()); 210 ASSERT(m_db.isOpen()); 202 211 203 212 m_iconIDForIconURLStatement = nullptr; … … 215 224 void IconDatabase::pruneTimerFired() 216 225 { 217 RELEASE_ASSERT(m_db.isOpen()); 226 ASSERT(!isMainThread()); 227 ASSERT(m_db.isOpen()); 218 228 219 229 if (!m_pruneIconsStatement) { … … 244 254 void IconDatabase::startPruneTimer() 245 255 { 256 ASSERT(!isMainThread()); 257 246 258 if (!m_pruneTimer || !m_db.isOpen()) 247 259 return; … … 254 266 void IconDatabase::clearLoadedIconsTimerFired() 255 267 { 268 ASSERT(isMainThread()); 269 270 LockHolder lockHolder(m_loadedIconsLock); 256 271 auto now = MonotonicTime::now(); 257 272 Vector<String> iconsToRemove; … … 270 285 void IconDatabase::startClearLoadedIconsTimer() 271 286 { 287 ASSERT(isMainThread()); 288 272 289 if (m_clearLoadedIconsTimer.isActive()) 273 290 return; … … 278 295 Optional<int64_t> IconDatabase::iconIDForIconURL(const String& iconURL, bool& expired) 279 296 { 280 RELEASE_ASSERT(m_db.isOpen()); 297 ASSERT(!isMainThread()); 298 ASSERT(m_db.isOpen()); 281 299 282 300 if (!m_iconIDForIconURLStatement) { … … 306 324 bool IconDatabase::setIconIDForPageURL(int64_t iconID, const String& pageURL) 307 325 { 308 RELEASE_ASSERT(m_db.isOpen()); 309 RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 326 ASSERT(!isMainThread()); 327 ASSERT(m_db.isOpen()); 328 ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 310 329 311 330 if (!m_setIconIDForPageURLStatement) { … … 333 352 Vector<char> IconDatabase::iconData(int64_t iconID) 334 353 { 335 RELEASE_ASSERT(m_db.isOpen()); 354 ASSERT(!isMainThread()); 355 ASSERT(m_db.isOpen()); 336 356 337 357 if (!m_iconDataStatement) { … … 359 379 Optional<int64_t> IconDatabase::addIcon(const String& iconURL, const Vector<char>& iconData) 360 380 { 361 RELEASE_ASSERT(m_db.isOpen()); 362 RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 381 ASSERT(!isMainThread()); 382 ASSERT(m_db.isOpen()); 383 ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 363 384 364 385 if (!m_addIconStatement) { … … 401 422 void IconDatabase::updateIconTimestamp(int64_t iconID, int64_t timestamp) 402 423 { 403 RELEASE_ASSERT(m_db.isOpen()); 404 RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 424 ASSERT(!isMainThread()); 425 ASSERT(m_db.isOpen()); 426 ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 405 427 406 428 if (!m_updateIconTimestampStatement) { … … 424 446 void IconDatabase::deleteIcon(int64_t iconID) 425 447 { 426 RELEASE_ASSERT(m_db.isOpen()); 427 RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 448 ASSERT(!isMainThread()); 449 ASSERT(m_db.isOpen()); 450 ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes); 428 451 429 452 if (!m_deletePageURLsForIconStatement) { … … 470 493 void IconDatabase::checkIconURLAndSetPageURLIfNeeded(const String& iconURL, const String& pageURL, AllowDatabaseWrite allowDatabaseWrite, CompletionHandler<void(bool, bool)>&& completionHandler) 471 494 { 495 ASSERT(isMainThread()); 496 472 497 m_workQueue->dispatch([this, protectedThis = makeRef(*this), iconURL = iconURL.isolatedCopy(), pageURL = pageURL.isolatedCopy(), allowDatabaseWrite, completionHandler = WTFMove(completionHandler)]() mutable { 473 498 bool result = false; … … 476 501 bool canWriteToDatabase = m_allowDatabaseWrite == AllowDatabaseWrite::Yes && allowDatabaseWrite == AllowDatabaseWrite::Yes; 477 502 bool expired = false; 478 if (m_pageURLToIconURLMap.get(pageURL) == iconURL) 503 String cachedIconURL; 504 { 505 LockHolder lockHolder(m_pageURLToIconURLMapLock); 506 cachedIconURL = m_pageURLToIconURLMap.get(pageURL); 507 } 508 if (cachedIconURL == iconURL) 479 509 result = true; 480 510 else if (auto iconID = iconIDForIconURL(iconURL, expired)) { … … 487 517 result = true; 488 518 if (!canWriteToDatabase || setIconIDForPageURL(iconID.value(), pageURL)) { 519 LockHolder lockHolder(m_pageURLToIconURLMapLock); 489 520 m_pageURLToIconURLMap.set(pageURL, iconURL); 490 521 changed = true; 491 522 } 492 523 } 493 } else if (!canWriteToDatabase && m_loadedIcons.contains(iconURL)) { 494 // Found in memory cache. 495 result = true; 496 m_pageURLToIconURLMap.set(pageURL, iconURL); 497 changed = true; 524 } else if (!canWriteToDatabase) { 525 bool foundInMemoryCache; 526 { 527 LockHolder lockHolder(m_loadedIconsLock); 528 foundInMemoryCache = m_loadedIcons.contains(iconURL); 529 } 530 531 if (foundInMemoryCache) { 532 result = true; 533 LockHolder lockHolder(m_pageURLToIconURLMapLock); 534 m_pageURLToIconURLMap.set(pageURL, iconURL); 535 changed = true; 536 } 498 537 } 499 538 } … … 507 546 void IconDatabase::loadIconForPageURL(const String& pageURL, AllowDatabaseWrite allowDatabaseWrite, CompletionHandler<void(NativeImagePtr&&)>&& completionHandler) 508 547 { 548 ASSERT(isMainThread()); 549 509 550 m_workQueue->dispatch([this, protectedThis = makeRef(*this), pageURL = pageURL.isolatedCopy(), allowDatabaseWrite, timestamp = WallTime::now().secondsSinceEpoch(), completionHandler = WTFMove(completionHandler)]() mutable { 510 551 Optional<int64_t> iconID; 511 552 Vector<char> iconData; 512 auto iconURL = m_pageURLToIconURLMap.get(pageURL); 553 String iconURL; 554 { 555 LockHolder lockHolder(m_pageURLToIconURLMapLock); 556 iconURL = m_pageURLToIconURLMap.get(pageURL); 557 } 513 558 if (m_db.isOpen() && !iconURL.isEmpty()) { 514 559 bool expired; 515 560 iconID = iconIDForIconURL(iconURL, expired); 516 if (iconID && !m_loadedIcons.contains(iconURL)) { 517 iconData = this->iconData(iconID.value()); 518 m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { })); 561 if (iconID) { 562 LockHolder lockHolder(m_loadedIconsLock); 563 if (!m_loadedIcons.contains(iconURL)) { 564 iconData = this->iconData(iconID.value()); 565 m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { })); 566 } 519 567 } 520 568 bool canWriteToDatabase = m_allowDatabaseWrite == AllowDatabaseWrite::Yes && allowDatabaseWrite == AllowDatabaseWrite::Yes; … … 529 577 } 530 578 579 LockHolder lockHolder(m_loadedIconsLock); 531 580 auto it = m_loadedIcons.find(iconURL); 532 581 if (it != m_loadedIcons.end() && it->value.first) { … … 534 583 it->value.second = MonotonicTime::now(); 535 584 startClearLoadedIconsTimer(); 585 lockHolder.unlockEarly(); 536 586 completionHandler(WTFMove(icon)); 537 587 return; … … 550 600 auto icon = addResult.iterator->value.first; 551 601 startClearLoadedIconsTimer(); 602 lockHolder.unlockEarly(); 552 603 completionHandler(WTFMove(icon)); 553 604 }); … … 557 608 String IconDatabase::iconURLForPageURL(const String& pageURL) 558 609 { 610 ASSERT(isMainThread()); 611 612 LockHolder lockHolder(m_pageURLToIconURLMapLock); 559 613 return m_pageURLToIconURLMap.get(pageURL); 560 614 } … … 562 616 void IconDatabase::setIconForPageURL(const String& iconURL, const unsigned char* iconData, size_t iconDataSize, const String& pageURL, AllowDatabaseWrite allowDatabaseWrite, CompletionHandler<void(bool)>&& completionHandler) 563 617 { 618 ASSERT(isMainThread()); 619 564 620 // If database write is not allowed load the icon to cache it in memory only. 565 621 if (m_allowDatabaseWrite == AllowDatabaseWrite::No || allowDatabaseWrite == AllowDatabaseWrite::No) { 566 622 bool result = true; 567 auto addResult = m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { })); 568 if (iconDataSize) { 569 auto image = BitmapImage::create(); 570 if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) < EncodedDataStatus::SizeAvailable) 571 result = false; 572 else 573 addResult.iterator->value.first = image->nativeImageForCurrentFrame(); 623 { 624 LockHolder lockHolder(m_loadedIconsLock); 625 auto addResult = m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { })); 626 if (iconDataSize) { 627 auto image = BitmapImage::create(); 628 if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) < EncodedDataStatus::SizeAvailable) 629 result = false; 630 else 631 addResult.iterator->value.first = image->nativeImageForCurrentFrame(); 632 } 574 633 } 575 634 startClearLoadedIconsTimer(); 576 635 m_workQueue->dispatch([this, protectedThis = makeRef(*this), result, iconURL = iconURL.isolatedCopy(), pageURL = pageURL.isolatedCopy(), completionHandler = WTFMove(completionHandler)]() mutable { 577 m_pageURLToIconURLMap.set(pageURL, iconURL); 636 { 637 LockHolder lockHolder(m_pageURLToIconURLMapLock); 638 m_pageURLToIconURLMap.set(pageURL, iconURL); 639 } 578 640 RunLoop::main().dispatch([result, completionHandler = WTFMove(completionHandler)]() mutable { 579 641 completionHandler(result); … … 599 661 if (iconID) { 600 662 result = true; 601 if (setIconIDForPageURL(iconID.value(), pageURL)) 663 if (setIconIDForPageURL(iconID.value(), pageURL)) { 664 LockHolder lockHolder(m_pageURLToIconURLMapLock); 602 665 m_pageURLToIconURLMap.set(pageURL, iconURL); 666 } 603 667 } 604 668 … … 614 678 void IconDatabase::clear(CompletionHandler<void()>&& completionHandler) 615 679 { 616 m_loadedIcons.clear(); 680 ASSERT(isMainThread()); 681 682 { 683 LockHolder lockHolder(m_loadedIconsLock); 684 m_loadedIcons.clear(); 685 } 617 686 m_workQueue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable { 618 m_pageURLToIconURLMap.clear(); 687 { 688 LockHolder lockHolder(m_pageURLToIconURLMapLock); 689 m_pageURLToIconURLMap.clear(); 690 } 619 691 620 692 if (m_db.isOpen() && m_allowDatabaseWrite == AllowDatabaseWrite::Yes) { 621 m_pageURLToIconURLMap.clear();622 623 693 clearStatements(); 624 694 m_db.clearAllTables(); -
trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h
r250518 r251362 25 25 #include <wtf/CompletionHandler.h> 26 26 #include <wtf/HashMap.h> 27 #include <wtf/Lock.h> 27 28 #include <wtf/ThreadSafeRefCounted.h> 28 29 #include <wtf/WorkQueue.h> … … 70 71 WebCore::SQLiteDatabase m_db; 71 72 HashMap<String, String> m_pageURLToIconURLMap; 73 Lock m_pageURLToIconURLMapLock; 72 74 HashMap<String, std::pair<WebCore::NativeImagePtr, MonotonicTime>> m_loadedIcons; 75 Lock m_loadedIconsLock; 73 76 74 77 std::unique_ptr<WebCore::SQLiteStatement> m_iconIDForIconURLStatement;
Note: See TracChangeset
for help on using the changeset viewer.