blink/bindings: Fix false-positive ASAN warning in ParkableString.

The following sequence is racy with the current ASAN checks in ParkableString,
on the main thread:

Park()
Lock()
ToString()
Unlock()

Park() poisons the string, ToString() unpoisons it, and Unlock() poisons it
again. If this last call happens while the compression is in progress, then this
is a use-after-poison.

This is not a real issue, merely an overaly eager poisoning, still making using
ASAN builds painful. Fix it by making sure the string stays unpoisoned during
compression.
Also adds a regression test.

Bug: 905137,877044
Change-Id: I5276b9ae6eee4abe2f2bf041818d1ba17358a80a
Reviewed-on: https://chromium-review.googlesource.com/c/1335585Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608289}

third_party/blink/renderer/platform/bindings/parkable_string.cc

@@ -176,7 +176,10 @@ void ParkableStringImpl::Unlock() {
   // the owning thread may concurrently call |ToString()|. It is then allowed
   // to use the string until the end of the current owning thread task.
   // Requires DCHECK_IS_ON() for the |owning_thread_| check.
-  if (CanParkNow() && owning_thread_ == CurrentThread()) {
+  //
+  // Checking the owning thread first as CanParkNow() can only be called from
+  // the owning thread.
+  if (owning_thread_ == CurrentThread() && CanParkNow()) {
     AsanPoisonString(string_);
   }
 #endif  // defined(ADDRESS_SANITIZER) && DCHECK_IS_ON()
@@ -318,6 +321,11 @@ void ParkableStringImpl::CompressInBackground(
                params->size);
 
   base::ElapsedTimer timer;
+#if defined(ADDRESS_SANITIZER)
+  // Lock the string to prevent a concurrent |Unlock()| on the main thread from
+  // poisoning the string in the meantime.
+  params->string->Lock();
+#endif  // defined(ADDRESS_SANITIZER)
   // Compression touches the string.
   AsanUnpoisonString(params->string->string_);
   base::StringPiece data(reinterpret_cast<const char*>(params->data),
@@ -332,7 +340,9 @@ void ParkableStringImpl::CompressInBackground(
         reinterpret_cast<const uint8_t*>(compressed_string.c_str()),
         compressed_string.size());
   }
-  AsanPoisonString(params->string->string_);
+#if defined(ADDRESS_SANITIZER)
+  params->string->Unlock();
+#endif  // defined(ADDRESS_SANITIZER)
 
   auto* task_runner = params->callback_task_runner.get();
   size_t size = params->size;

third_party/blink/renderer/platform/bindings/parkable_string_test.cc

@@ -410,6 +410,19 @@ TEST_F(ParkableStringTest, AsanPoisoningTest) {
   EXPECT_TRUE(ParkAndWait(parkable));
   EXPECT_DEATH(EXPECT_NE(0, data[10]), "");
 }
+
+// Non-regression test for crbug.com/905137.
+TEST_F(ParkableStringTest, CorrectAsanPoisoning) {
+  ParkableString parkable(MakeLargeString().ReleaseImpl());
+  EXPECT_TRUE(parkable.Impl()->Park());
+  // A main thread task is posted once compression is done.
+  while (!scoped_task_environment_.MainThreadHasPendingTask()) {
+    parkable.Lock();
+    parkable.ToString();
+    parkable.Unlock();
+  }
+  RunPostedTasks();
+}
 #endif  // defined(ADDRESS_SANITIZER)
 
 TEST_F(ParkableStringTest, Compression) {