Bug 108880

Summary: Create a TextResourceDecoder on the BackgroundHTMLParser
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, ap, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106127, 107603    
Attachments:
Description Flags
Patch buildbot: commit-queue-

Tony Gentilcore
Reported 2013-02-04 17:13:40 PST
Create a TextResourceDecoder on the BackgroundHTMLParser
Attachments
Patch (4.58 KB, patch)
2013-02-04 17:17 PST, Tony Gentilcore 		buildbot: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Tony Gentilcore
Comment 1 2013-02-04 17:17:20 PST
Created attachment 186504 [details] Patch
Adam Barth
Comment 2 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.
Alexey Proskuryakov
Comment 3 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.
Build Bot
Comment 4 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
Build Bot
Comment 5 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
Adam Barth
Comment 6 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?
Alexey Proskuryakov
Comment 7 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.
Adam Barth
Comment 8 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.
Tony Gentilcore
Comment 9 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.
Alexey Proskuryakov
Comment 10 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.
Adam Barth
Comment 11 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.
Alexey Proskuryakov
Comment 12 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.
Adam Barth
Comment 13 2013-02-06 16:29:13 PST
There's some further discussion in bug 107603 that might interest you as well.
Note You need to log in before you can comment on or make changes to this bug.