Create a TextResourceDecoder on the BackgroundHTMLParser

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Tony Gentilcore]

Create a TextResourceDecoder on the BackgroundHTMLParser

[Tony Gentilcore]

Created attachment 186504 [details]
Patch

[Adam Barth]

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]

I don't think that this is safe. atomicCanonicalTextEncodingName() calls functions like buildBaseTextCodecMaps() and extendTextCodecMaps(), which are main thread only.

[Build Bot]

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 on attachment 186504 [details]
Patch

Attachment 186504 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16369663

[Adam Barth]

(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]

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]

> 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]

I'm not going to do this as a separate patch after all. Please see the complete thing on bug 107603.

[Alexey Proskuryakov]

Can you respond to my comments? The patch in bug 107603 is large, and I'm unsure how it answers them.

[Adam Barth]

> 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]

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]

There's some further discussion in bug 107603 that might interest you as well.