Bug 108880 - Create a TextResourceDecoder on the BackgroundHTMLParser
Summary: Create a TextResourceDecoder on the BackgroundHTMLParser
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 106127 107603
  Show dependency treegraph
 
Reported: 2013-02-04 17:13 PST by Tony Gentilcore
Modified: 2013-02-06 16:29 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.58 KB, patch)
2013-02-04 17:17 PST, Tony Gentilcore
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2013-02-04 17:13:40 PST
Create a TextResourceDecoder on the BackgroundHTMLParser
Comment 1 Tony Gentilcore 2013-02-04 17:17:20 PST
Created attachment 186504 [details]
Patch
Comment 2 Adam Barth 2013-02-04 18:44:34 PST
Comment on attachment 186504 [details]
Patch

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

> Source/WebCore/html/parser/HTMLParserOptions.h:44
> +    String mimeType;
> +    String defaultTextEncodingName;

Now HTMLParserOption isn't thread-safe.  Presumably we need to account for that somewhere.
Comment 3 Alexey Proskuryakov 2013-02-04 23:37:10 PST
I don't think that this is safe. atomicCanonicalTextEncodingName() calls functions like buildBaseTextCodecMaps() and extendTextCodecMaps(), which are main thread only.
Comment 4 Build Bot 2013-02-05 01:58:40 PST
Comment on attachment 186504 [details]
Patch

Attachment 186504 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16373569
Comment 5 Build Bot 2013-02-05 02:58:50 PST
Comment on attachment 186504 [details]
Patch

Attachment 186504 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16369663
Comment 6 Adam Barth 2013-02-05 13:14:51 PST
(In reply to comment #3)
> I don't think that this is safe. atomicCanonicalTextEncodingName() calls functions like buildBaseTextCodecMaps() and extendTextCodecMaps(), which are main thread only.

Is MutexLocker lock(encodingRegistryMutex()); not sufficient?
Comment 7 Alexey Proskuryakov 2013-02-05 13:56:52 PST
buildBaseTextCodecMaps() has an ASSERT(isMainThread()) in it, so no, not quite sufficient.

I don't remember much detail, but this code is a bit more fragile than it should be. It was particularly easy to get into a recursive deadlock by adding too many locks.

We only have a mutex around codec registry - but actual codecs do interesting things threading-wise - see e.g. cachedConverterICU() in TextCodecICU.cpp. The idea here is that we have non-WebCore callers on secondary threads, but callers are tightly bound to their thread.
Comment 8 Adam Barth 2013-02-05 15:37:24 PST
> buildBaseTextCodecMaps() has an ASSERT(isMainThread()) in it, so no, not quite sufficient.

From reading the code, it seems like that ASSERT just ensures that we build the codec map on the main thread.  After it is built, it looks to be useable from any thread.  I haven't studied the code in detail, however.
Comment 9 Tony Gentilcore 2013-02-06 15:03:05 PST
I'm not going to do this as a separate patch after all. Please see the complete thing on bug 107603.
Comment 10 Alexey Proskuryakov 2013-02-06 16:08:18 PST
Can you respond to my comments? The patch in bug 107603 is large, and I'm unsure how it answers them.
Comment 11 Adam Barth 2013-02-06 16:11:11 PST
> Can you respond to my comments? The patch in bug 107603 is large, and I'm unsure how it answers them.

The patch in bug 107603 calls TextResourceDecoder::create on the main thread.
Comment 12 Alexey Proskuryakov 2013-02-06 16:27:20 PST
Thank you for the explanation, that certainly resolves the concern. I misread comment 9 as implying that the same approach is part of the patch in bug 107603.
Comment 13 Adam Barth 2013-02-06 16:29:13 PST
There's some further discussion in bug 107603 that might interest you as well.