Changeset 101813 in webkit

Timestamp:

Dec 2, 2011 9:42:54 AM (12 years ago)

Author:

Darin Adler

Message:

[Mac] Reference count threading violation in FormDataStreamMac.mm
​https://bugs.webkit.org/show_bug.cgi?id=73627

Reviewed by Sam Weinig.

Shows up as a crash during existing layout test runs so no new tests are required.

• platform/network/mac/FormDataStreamMac.mm:

(WebCore::streamFieldsMap): Replaced getStreamFormDataMap with this.
Use an NSMapTable instead of a HashMap because we need to remove items from this
on a non-main thread.
(WebCore::associateStreamWithResourceHandle): Use NSMapGet instead of
HashMap::contains here.
(WebCore::formCreate): FormStreamFields now stores a RefPtr to the form data.
Added the code to fill that in. Did it in a more modern way to avoid the leakRef
and adoptRef that were used before. Replaced the code that set up the stream
form data map entry with code that sets an entry in the streamFieldsMap.
(WebCore::formFinishFinalizationOnMainThread): Added. Contains the work of
finalization that must be done on the main thread, specifically, destroying the
fields structure that contains objects with RefPtr in them. We can't touch these
reference counts on non-main threads.
(WebCore::formFinalize): Changed this to use NSMapRemove on the streamFieldsMap.
Added a callOnMainThread to finish the finalization.
(WebCore::setHTTPBody): Removed the leakRef, no longer needed, that used to be
balanced by an adoptRef in formCreate.
(WebCore::httpBodyFromStream): Changed to use NSMapGet.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/network/mac/FormDataStreamMac.mm (modified) (8 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-12-01  Darin Adler  <darin@apple.com>
+
+        [Mac] Reference count threading violation in FormDataStreamMac.mm
+        https://bugs.webkit.org/show_bug.cgi?id=73627
+
+        Reviewed by Sam Weinig.
+
+        Shows up as a crash during existing layout test runs so no new tests are required.
+
+        * platform/network/mac/FormDataStreamMac.mm:
+        (WebCore::streamFieldsMap): Replaced getStreamFormDataMap with this.
+        Use an NSMapTable instead of a HashMap because we need to remove items from this
+        on a non-main thread.
+        (WebCore::associateStreamWithResourceHandle): Use NSMapGet instead of
+        HashMap::contains here.
+        (WebCore::formCreate): FormStreamFields now stores a RefPtr to the form data.
+        Added the code to fill that in. Did it in a more modern way to avoid the leakRef
+        and adoptRef that were used before. Replaced the code that set up the stream
+        form data map entry with code that sets an entry in the streamFieldsMap.
+        (WebCore::formFinishFinalizationOnMainThread): Added. Contains the work of
+        finalization that must be done on the main thread, specifically, destroying the
+        fields structure that contains objects with RefPtr in them. We can't touch these
+        reference counts on non-main threads.
+        (WebCore::formFinalize): Changed this to use NSMapRemove on the streamFieldsMap.
+        Added a callOnMainThread to finish the finalization.
+        (WebCore::setHTTPBody): Removed the leakRef, no longer needed, that used to be
+        balanced by an adoptRef in formCreate.
+        (WebCore::httpBodyFromStream): Changed to use NSMapGet.
+
 2011-12-02  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebCore/platform/network/mac/FormDataStreamMac.mm

@@ -1 +1 @@
 /*
- * Copyright (C) 2005, 2006, 2008 Apple Inc.  All rights reserved.
+ * Copyright (C) 2005, 2006, 2008, 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -57 +57 @@
 namespace WebCore {
 
-typedef HashMap<CFReadStreamRef, RefPtr<FormData> > StreamFormDataMap;
-static StreamFormDataMap& getStreamFormDataMap()
-{
-    DEFINE_STATIC_LOCAL(StreamFormDataMap, streamFormDataMap, ());
-    return streamFormDataMap;
+// We need to use NSMapTable instead of HashMap since this needs to be accessed from more than one thread.
+static NSMapTable *streamFieldsMap()
+{
+    static NSMapTable *streamFieldsMap = NSCreateMapTable(NSNonRetainedObjectMapKeyCallBacks, NSNonOwnedPointerMapValueCallBacks, 1);
+    return streamFieldsMap;
 }
 
@@ -80 +80 @@
         return;
 
-    if (!getStreamFormDataMap().contains((CFReadStreamRef)stream))
+    if (!NSMapGet(streamFieldsMap(), stream))
         return;
 
@@ -126 +126 @@
 
 struct FormContext {
-    FormData* formData;
+    RefPtr<FormData> formData;
     unsigned long long streamLength;
 };
 
 struct FormStreamFields {
+    RefPtr<FormData> formData;
     SchedulePairHashSet scheduledRunLoopPairs;
     Vector<FormDataElement> remainingElements; // in reverse order
@@ -231 +232 @@
 
     FormStreamFields* newInfo = new FormStreamFields;
+    newInfo->formData = formContext->formData.release();
     newInfo->currentStream = NULL;
 #if ENABLE(BLOB)
@@ -240 +242 @@
     newInfo->bytesSent = 0;
 
-    FormData* formData = formContext->formData;
-
     // Append in reverse order since we remove elements from the end.
-    size_t size = formData->elements().size();
+    size_t size = newInfo->formData->elements().size();
     newInfo->remainingElements.reserveInitialCapacity(size);
     for (size_t i = 0; i < size; ++i)
-        newInfo->remainingElements.append(formData->elements()[size - i - 1]);
-
-    getStreamFormDataMap().set(stream, adoptRef(formData));
+        newInfo->remainingElements.append(newInfo->formData->elements()[size - i - 1]);
+
+    ASSERT(!NSMapGet(streamFieldsMap(), stream));
+    NSMapInsertKnownAbsent(streamFieldsMap(), stream, newInfo);
 
     return newInfo;
 }
 
+static void formFinishFinalizationOnMainThread(void* context)
+{
+    OwnPtr<FormStreamFields> form = adoptPtr(static_cast<FormStreamFields*>(context));
+
+    closeCurrentStream(form.get());
+}
+
 static void formFinalize(CFReadStreamRef stream, void* context)
 {
-    OwnPtr<FormStreamFields> form = adoptPtr(static_cast<FormStreamFields*>(context));
-
-    getStreamFormDataMap().remove(stream);
-
-    closeCurrentStream(form.get());
+    FormStreamFields* form = static_cast<FormStreamFields*>(context);
+
+    ASSERT(form->formStream == stream);
+    ASSERT(NSMapGet(streamFieldsMap(), stream) == context);
+
+    // Do this right away because the CFReadStreamRef is being deallocated.
+    // We can't wait to remove this from the map until we finish finalizing
+    // on the main thread because in theory the freed memory could be reused
+    // for a new CFReadStream before that runs.
+    NSMapRemove(streamFieldsMap(), stream);
+
+    callOnMainThread(formFinishFinalizationOnMainThread, form);
 }
 
@@ -472 +487 @@
 
     // Pass the length along with the formData so it does not have to be recomputed.
-    FormContext formContext = { formData.release().leakRef(), length };
+    FormContext formContext = { formData.release(), length };
 
     RetainPtr<CFReadStreamRef> stream(AdoptCF, wkCreateCustomCFReadStream(formCreate, formFinalize,
@@ -482 +497 @@
 FormData* httpBodyFromStream(NSInputStream* stream)
 {
-    return getStreamFormDataMap().get((CFReadStreamRef)stream).get();
+    FormStreamFields* formStream = static_cast<FormStreamFields*>(NSMapGet(streamFieldsMap(), stream));
+    if (!formStream)
+        return 0;
+    return formStream->formData.get();
 }