Bug 133936 - API::String is not thread-safe
Summary: API::String is not thread-safe
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords:
Depends on: 139261
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-15 23:40 PDT by mitz
Modified: 2014-12-04 17:08 PST (History)
1 user (show)

See Also:


Attachments
Safely dispose of m_string on the main thread (3.62 KB, patch)
2014-06-16 00:09 PDT, mitz
no flags Details | Formatted Diff | Diff
Safely dispose of m_string on the main thread (3.76 KB, patch)
2014-06-16 22:16 PDT, mitz
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2014-06-15 23:40:09 PDT
API::String is not thread-safe
Comment 1 mitz 2014-06-16 00:09:13 PDT
Created attachment 233152 [details]
Safely dispose of m_string on the main thread
Comment 2 WebKit Commit Bot 2014-06-16 00:11:33 PDT
Attachment 233152 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/APIString.h:133:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Anders Carlsson 2014-06-16 16:11:18 PDT
Comment on attachment 233152 [details]
Safely dispose of m_string on the main thread

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

> Source/WebKit2/Shared/APIString.h:68
> +        WTF::String string = std::move(m_string);

Can use auto here.

> Source/WebKit2/Shared/APIString.h:69
> +        RunLoop::main().dispatch([string]() { });

I think this needs a comment.

> Source/WebKit2/Shared/APIString.h:115
> +        RunLoop::main().dispatch([string]() { });

I think this needs a comment.
Comment 4 mitz 2014-06-16 22:16:23 PDT
Created attachment 233218 [details]
Safely dispose of m_string on the main thread
Comment 5 WebKit Commit Bot 2014-06-16 22:18:55 PDT
Attachment 233218 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/APIString.h:135:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Anders Carlsson 2014-06-18 08:24:58 PDT
Comment on attachment 233218 [details]
Safely dispose of m_string on the main thread

What if the string is created off the main thread?
Comment 7 mitz 2014-06-18 08:43:44 PDT
(In reply to comment #6)
> (From update of attachment 233218 [details])
> What if the string is created off the main thread?

In that case we make an isolated copy at creation time, so we are free to dispose of it on the main thread eventually. The invariant is that m_string is always safe to destroy on the main thread.
Comment 8 mitz 2014-06-18 08:52:12 PDT
Oh, I think you're saying that the string() accessor may violate the invariant. It should just make an isolated copy and return it whenever it's called off the main thread.
Comment 9 mitz 2014-12-04 17:08:12 PST
The fix for bug 139261 made API::String copy unconditionally on creation.