Moved text decoding to the parser thread

Moved text decoding to the parser thread.

When the HTTPDocumentParser defers the document parsing to the background parser thread, it'll now also pass ownership of its TextResourceDecoder so the parser thread can do the decoding as well.

R=abarth@chromium.org,eseidel@chromium.org
BUG=277886
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163977

[oystein (OOO til 10th of July), 2013-11-16 01:24:58]

Please take a look and let me know if you agree with the approach or not.

The final step after this would be to route the raw bytes directly to the parser
thread without going through the main thread, once we've set everything up.

[abarth-chromium, 2013-11-18 06:57:10]

I probably should look at this CL again when I'm less tired.  Here are some
random notes.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
File Source/core/html/parser/BackgroundHTMLParser.cpp (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:29: #include
"core/fetch/TextResourceDecoder.h"
Should this class move to the core/html/parser directory?

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
File Source/core/html/parser/BackgroundHTMLParser.h (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.h:55: static void
create(PassRefPtr<WeakReference<BackgroundHTMLParser> >,
PassOwnPtr<Configuration>, PassOwnPtr<TextResourceDecoder>);
Why not put the TextResourceDecoder in the Configuration?  It exists to hold all
the parameters that this function needs.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.cpp:323: void
HTMLDocumentParser::didReceiveEncodingDataFromBackgroundParser(const
DocumentEncodingData& data)
Missing a blank line above this line.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.cpp:979:
HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::appendBytes,
m_backgroundParser, SharedBuffer::create(data, length)));
Why a SharedBuffer and not just a Vector?  It's too bad we're introducing this
extra copy.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.h:99: void
didReceiveEncodingDataFromBackgroundParser(const DocumentEncodingData&);
Can we pass by value instead of by const reference?  I'm not sure WTF::bind is
smart enough to handle references correctly.

[abarth-chromium, 2013-11-18 17:32:23]

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
File Source/core/dom/DecodedDataDocumentParser.h (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
Source/core/dom/DecodedDataDocumentParser.h:36: class DecodedDataDocumentParser
: public DocumentParser {
I wonder if we should restructure how this class works.  The point of this class
before was to provide decoded data to its subclasses.  Now that you've got
HTMLDocumentParser overriding appendBytes, we should probably restructure how
this works.  What are the subclasses of this class?  Is it just the
HTMLDocumentParser and XMLDocumentParser?  If so, they probably should "have" an
object that manages the decoding rather than "being" an object that manages the
decoding.

[eseidel, 2013-11-18 20:07:39]

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
File Source/core/dom/DecodedDataDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
Source/core/dom/DecodedDataDocumentParser.cpp:57:
PassOwnPtr<TextResourceDecoder> DecodedDataDocumentParser::adoptDecoder()
Maybe takeDecoder()?  Not quite sure what this is for yet...

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
File Source/core/html/parser/BackgroundHTMLParser.cpp (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:85: new
BackgroundHTMLParser(reference, config, decoder);
This reads very strange.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:86: // Caller must free by
calling stop().
If this is deleted via "stop" maybe it should be created via start()?

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:123: String remainingData =
m_decoder->flush();
I'm not sure the local buys you much.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:130: encodingData.encoding =
m_decoder->encoding();
This feels like DocumentEndodingData should be something the Decoder knows how
to return for you?  Or maybe DocumentEncodingData has a constructor which knows
how to fill itself from a Decoder?  This just doesn't feel like it belongs here.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
File Source/core/html/parser/BackgroundHTMLParser.h (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.h:55: static void
create(PassRefPtr<WeakReference<BackgroundHTMLParser> >,
PassOwnPtr<Configuration>, PassOwnPtr<TextResourceDecoder>);
On 2013/11/18 06:57:10, abarth wrote:
> Why not put the TextResourceDecoder in the Configuration?  It exists to hold
all
> the parameters that this function needs.

+1

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
File Source/core/html/parser/HTMLDocumentParser.cpp (left):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.cpp:701: TRACE_EVENT1("net",
"HTMLDocumentParser::append", "size", inputSource->length());
We've lost this trace event now.  I'm not sure that matters.

[oystein (OOO til 10th of July), 2013-11-18 22:18:42]

Thanks guys :)

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
File Source/core/dom/DecodedDataDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
Source/core/dom/DecodedDataDocumentParser.cpp:57:
PassOwnPtr<TextResourceDecoder> DecodedDataDocumentParser::adoptDecoder()
On 2013/11/18 20:07:40, eseidel wrote:
> Maybe takeDecoder()?  Not quite sure what this is for yet...

Done.

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
File Source/core/dom/DecodedDataDocumentParser.h (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/dom/DecodedDataDo...
Source/core/dom/DecodedDataDocumentParser.h:36: class DecodedDataDocumentParser
: public DocumentParser {
On 2013/11/18 17:32:24, abarth wrote:
> I wonder if we should restructure how this class works.  The point of this
class
> before was to provide decoded data to its subclasses.  Now that you've got
> HTMLDocumentParser overriding appendBytes, we should probably restructure how
> this works.  What are the subclasses of this class?  Is it just the
> HTMLDocumentParser and XMLDocumentParser?  If so, they probably should "have"
an
> object that manages the decoding rather than "being" an object that manages
the
> decoding.

It's used by those two plus the HTMLViewSourceParser, as far as I can tell. I'll
gladly shuffle this to be a component if it's still worth it.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
File Source/core/html/parser/BackgroundHTMLParser.cpp (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:29: #include
"core/fetch/TextResourceDecoder.h"
On 2013/11/18 06:57:10, abarth wrote:
> Should this class move to the core/html/parser directory?

It does live in an awkward location right now, but it handles more than html
too. Then again core/html/parser/ does have other non-HTML things like the CSS
preload scanner, so maybe that's OK?

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:85: new
BackgroundHTMLParser(reference, config, decoder);
On 2013/11/18 20:07:40, eseidel wrote:
> This reads very strange.

I agree (I just moved this from the header), I don't see any big benefits to
using the WeakPtrFactory thing here. Should I just yank it out?

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:86: // Caller must free by
calling stop().
On 2013/11/18 20:07:40, eseidel wrote:
> If this is deleted via "stop" maybe it should be created via start()?

Done.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:123: String remainingData =
m_decoder->flush();
On 2013/11/18 20:07:40, eseidel wrote:
> I'm not sure the local buys you much.

error: non-const lvalue reference to type 'WTF::String' cannot bind to a
temporary of type 'WTF::String'

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/Backg...
Source/core/html/parser/BackgroundHTMLParser.cpp:130: encodingData.encoding =
m_decoder->encoding();
On 2013/11/18 20:07:40, eseidel wrote:
> This feels like DocumentEndodingData should be something the Decoder knows how
> to return for you?  Or maybe DocumentEncodingData has a constructor which
knows
> how to fill itself from a Decoder?  This just doesn't feel like it belongs
here.

Done; Added TextResourceDecoder::getEncodingData().

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
File Source/core/html/parser/HTMLDocumentParser.cpp (left):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.cpp:701: TRACE_EVENT1("net",
"HTMLDocumentParser::append", "size", inputSource->length());
On 2013/11/18 20:07:40, eseidel wrote:
> We've lost this trace event now.  I'm not sure that matters.

Added to appendBytes() now, just in case.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.cpp:323: void
HTMLDocumentParser::didReceiveEncodingDataFromBackgroundParser(const
DocumentEncodingData& data)
On 2013/11/18 06:57:10, abarth wrote:
> Missing a blank line above this line.

Done.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.cpp:979:
HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::appendBytes,
m_backgroundParser, SharedBuffer::create(data, length)));
On 2013/11/18 06:57:10, abarth wrote:
> Why a SharedBuffer and not just a Vector?  It's too bad we're introducing this
> extra copy.

I didn't see any other buffer classes when I looked; Vector definitely makes
sense. Done.

This copy just replaces the one done in HTMLDocumentParser::append() (if I'm
reading it right) , but would still be nice to avoid. Maybe the appendBytes()
could be switched to passing around a SharedBuffer argument instead, but I don't
know the full consequences of that.

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.h:99: void
didReceiveEncodingDataFromBackgroundParser(const DocumentEncodingData&);
On 2013/11/18 06:57:10, abarth wrote:
> Can we pass by value instead of by const reference?  I'm not sure WTF::bind is
> smart enough to handle references correctly.

Hm AFAIK normally functors only accept const-versions of reference arguments
precisely so it can make a local copy, but I'll double-check this.

[oystein (OOO til 10th of July), 2013-11-18 22:31:02]

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/74513003/diff/1/Source/core/html/parser/HTMLD...
Source/core/html/parser/HTMLDocumentParser.h:99: void
didReceiveEncodingDataFromBackgroundParser(const DocumentEncodingData&);
On 2013/11/18 22:18:42, Oystein wrote:
> On 2013/11/18 06:57:10, abarth wrote:
> > Can we pass by value instead of by const reference?  I'm not sure WTF::bind
is
> > smart enough to handle references correctly.
> 
> Hm AFAIK normally functors only accept const-versions of reference arguments
> precisely so it can make a local copy, but I'll double-check this.

Yep the type trait at wtf/Functional.h:325 should handle this correctly; it'll
take a local copy of the struct.

[eseidel, 2013-11-18 22:32:07]

https://codereview.chromium.org/74513003/diff/130001/Source/core/dom/DecodedD...
File Source/core/dom/DecodedDataDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/130001/Source/core/dom/DecodedD...
Source/core/dom/DecodedDataDocumentParser.cpp:102:
m_decoder->getEncodingData(encodingData);
document()->setEncodingData(m_decoder->encodingData())?

https://codereview.chromium.org/74513003/diff/130001/Source/core/fetch/TextRe...
File Source/core/fetch/TextResourceDecoder.h (right):

https://codereview.chromium.org/74513003/diff/130001/Source/core/fetch/TextRe...
Source/core/fetch/TextResourceDecoder.h:67: void
getEncodingData(DocumentEncodingData&) const;
"get" methods are uncommon in Blink.  Is there a reason this needs to use an out
param?  Is DocumentEncodingData non-copyable?

[oystein (OOO til 10th of July), 2013-11-18 22:36:46]

https://codereview.chromium.org/74513003/diff/130001/Source/core/fetch/TextRe...
File Source/core/fetch/TextResourceDecoder.h (right):

https://codereview.chromium.org/74513003/diff/130001/Source/core/fetch/TextRe...
Source/core/fetch/TextResourceDecoder.h:67: void
getEncodingData(DocumentEncodingData&) const;
On 2013/11/18 22:32:07, eseidel wrote:
> "get" methods are uncommon in Blink.  Is there a reason this needs to use an
out
> param?  Is DocumentEncodingData non-copyable?

It's copyable, it'd just add the need for the full definition of
DocumentEncodingData in TextResourceDecoder.h and I try to avoid that in
general. If that's the Blink way I have no problem with it though.

[eseidel, 2013-11-18 22:40:26]

We could also go the other way, and be able to fill one of these from a
Decoder*.

It's not a big deal one way or the other.

[oystein (OOO til 10th of July), 2013-11-19 22:07:30]

On 2013/11/18 22:40:26, eseidel wrote:
> We could also go the other way, and be able to fill one of these from a
> Decoder*.
> 
> It's not a big deal one way or the other.

Yeah I think I like that better then; done.


Also: What should we do with DecodedDataDocumentParser, as per prior comments?

[eseidel, 2013-11-19 22:27:45]

looks great.  Just a few nits.  Update the description and I think we're ready
to rock and roll.

https://codereview.chromium.org/74513003/diff/280001/Source/core/fetch/TextRe...
File Source/core/fetch/TextResourceDecoder.h (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/fetch/TextRe...
Source/core/fetch/TextResourceDecoder.h:87: 
Intentional?

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
File Source/core/html/parser/BackgroundHTMLParser.cpp (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.cpp:117: updateDocument(decoded);
The local still isnt' needed. :)

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.cpp:123:
updateDocument(remainingData);
Or here.

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.cpp:126: void
BackgroundHTMLParser::updateDocument(String& decodedData)
Why is this a ref?  Why not either by-value or a const ref?

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:982: TRACE_EVENT1("net",
"HTMLDocumentParser::appendBytes", "size", (unsigned)length);
Did you mean for this only to be in the threaded path?  I guess it was
previously... odd.

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.h:101: virtual void appendBytes(const
char* bytes, size_t length) OVERRIDE;
I thought you changed this to Vector<char>?  I guess not in this case?

[oystein (OOO til 10th of July), 2013-11-19 22:52:19]

Awesome, thanks :).

Note that this'll break support for the Visual Hebrew encoding; I didn't bother
implementing support for handling that on the parser thread, so please let me
know if I should  (I can just add it to the DocumentEncodingData) or whatever is
decided 

...though interestingly I see no visual different on
http://www.harel-net.co.il/takanon.html even without this particular part of the
support...

https://codereview.chromium.org/74513003/diff/280001/Source/core/fetch/TextRe...
File Source/core/fetch/TextResourceDecoder.h (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/fetch/TextRe...
Source/core/fetch/TextResourceDecoder.h:87: 
On 2013/11/19 22:27:45, eseidel wrote:
> Intentional?

Whoops. Nope.

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
File Source/core/html/parser/BackgroundHTMLParser.cpp (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.cpp:117: updateDocument(decoded);
On 2013/11/19 22:27:45, eseidel wrote:
> The local still isnt' needed. :)

Done.

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.cpp:123:
updateDocument(remainingData);
On 2013/11/19 22:27:45, eseidel wrote:
> Or here.

Done.

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.cpp:126: void
BackgroundHTMLParser::updateDocument(String& decodedData)
On 2013/11/19 22:27:45, eseidel wrote:
> Why is this a ref?  Why not either by-value or a const ref?

No reason in this case, just missed it after copying from DecodedDocumentParser.
Fixed :).

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:982: TRACE_EVENT1("net",
"HTMLDocumentParser::appendBytes", "size", (unsigned)length);
On 2013/11/19 22:27:45, eseidel wrote:
> Did you mean for this only to be in the threaded path?  I guess it was
> previously... odd.

Yeah there's an equivalent trace for the non-threaded path in
HTMLDocumentParser::append(). No idea if it's useful or not.

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/74513003/diff/280001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.h:101: virtual void appendBytes(const
char* bytes, size_t length) OVERRIDE;
On 2013/11/19 22:27:45, eseidel wrote:
> I thought you changed this to Vector<char>?  I guess not in this case?

I just changed the buffer we pass to the BackgroundHTMLParser::appendBytes from
a SharedBuffer to a Vector<char>, not the appendBytes() in the "main" parser
hierarchy. I wouldn't mind having a look at changing that, but I suspect it may
be a bigger operation which may make more sense in a separate patch?

It means we're still doing a copy of the incoming data which would ideally be
avoided, but we were under the old codepath as well.

[eseidel, 2013-11-19 23:09:12]

I don't think we can break Visual Hebrew yet.  I believe I've lost that battle
for now.

[oystein (OOO til 10th of July), 2013-11-21 21:11:13]

On 2013/11/19 23:09:12, eseidel wrote:
> I don't think we can break Visual Hebrew yet.  I believe I've lost that battle
> for now.

I've fixed this now, and isolated the support to be contained in just
Document::setEncodingData. Makes more sense to me now; it'll keep working even
if the encoding is set after the first chunk.

I've encountered another issue though: the XSS auditor. XSSAuditor::init assumes
that the encoding is available at that point, which with the current patch isn't
true anymore. That's fixable enough, but it seems to me this also relies on the
encoding being set in the first chunk received..?

[eseidel, 2013-11-21 22:14:47]

I remembered this morning that I think TextDecoder buffers appends until it has
hit a limit or found an encoding.  So I think the "first packet" that the parser
sees will always have an encoding determined?

[oystein (OOO til 10th of July), 2013-11-21 22:24:33]

On 2013/11/21 22:14:47, eseidel wrote:
> I remembered this morning that I think TextDecoder buffers appends until it
has
> hit a limit or found an encoding.  So I think the "first packet" that the
parser
> sees will always have an encoding determined?

Ah looks like you're right, that makes things a bit safer :).

My latest version has the XSSAuditor working again (tests passing), in either
case.

[oystein (OOO til 10th of July), 2013-11-25 19:52:11]

ping :)

[eseidel, 2013-11-25 19:57:13]

lgtm.  I think abarth should probably look as well.  Since he and I have
somewhat split expertise around the various parts of the parser.

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3972: m_visuallyOrdered = !m_visuallyOrdered;
This reads weird to me, but OK.

[abarth-chromium, 2013-11-25 21:45:39]

not lgtm

This CL is very close to correct, but I'm not sure we've fully validated that
TextResourceDecoder is thread-safe.

For example, TextResourceDecoder depends on HTMLMetaCharsetParser and
HTMLMetaCharsetParser uses an AtomicString on line 181 of
HTMLMetaCharsetParser.cpp.  We're not allowed to use AtomicString on the parser
thread.

Similarly, as a result of this CL, DOMImplementation::isXMLMIMEType might be
called on the parser thread, but I doubt anyone working in DOMImplementation.cpp
would think that the code in that file would be used on any thread other than
the main thread.

In the first case, we should break the dependency from HTMLMetaCharsetParser on
AtomicString and use HTMLIdentifier instead.  For the second case, we should
move DOMImplementation::isXMLMIMEType to some other file that contains
cross-thread code.

There might be more issues similar to the ones list above.  We should study
TextResourceDecoder and its dependencies to make sure there aren't any threading
problems.

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
File Source/core/dom/DocumentEncodingData.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
Source/core/dom/DocumentEncodingData.cpp:31: 
You've got an extra blank line here.

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:981: memcpy(buffer->data(), data,
length);
It's too bad we end up with an extra memcpy here.  I guess this will go away
once the network stack can give us the bytes directly on the background thread.

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditor.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:310: if (!m_httpBodyAsString.isEmpty()) {
Should we clear out  m_httpBodyAsString after this runs?

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:737: &&
m_decodedHTTPBody.isSafeToSendToAnotherThread();
Don't we need to add m_httpBodyAsString to this list?

[oystein (OOO til 10th of July), 2013-11-25 22:07:38]

On 2013/11/25 21:45:39, abarth wrote:
> For example, TextResourceDecoder depends on HTMLMetaCharsetParser and
> HTMLMetaCharsetParser uses an AtomicString on line 181 of
> HTMLMetaCharsetParser.cpp.  We're not allowed to use AtomicString on the
parser
> thread.


I thought AtomicStrings used TLS for its string table, and the idea is that they
can be created and used on any thread, as long as instances don't cross any
thread boundaries?

Maybe it would make sense to add a isSafeToSendToAnotherThread() to the
TextResourceDecoder which checks all string members and propagates the call to
any members, to make sure we don't break the AtomicString contract anywhere?

[abarth-chromium, 2013-11-25 22:12:35]

On 2013/11/25 22:07:38, Oystein wrote:
> I thought AtomicStrings used TLS for its string table, and the idea is that
they
> can be created and used on any thread, as long as instances don't cross any
> thread boundaries?

That's correct, but we don't have an AtomicString table on the parser thread and
we don't want to create one.  We should probably have a DCHECK somewhere to
prevent it from slipping in.

> Maybe it would make sense to add a isSafeToSendToAnotherThread() to the
> TextResourceDecoder which checks all string members and propagates the call to
> any members, to make sure we don't break the AtomicString contract anywhere?

That tends to work for small objects like structs, but not for something as
complicated as the TextResourceDecoder.  Instead, we might want to move it and
all of its dependencies into a directory and then remember to worry about
threading when working in that directory.  For example, we could move them to
Source/platform/textdecoder.  (We can leave the lower-level code in WTF.)

[abarth-chromium, 2013-11-25 22:13:13]

On 2013/11/25 22:12:35, abarth wrote:
> For example, we could move them to Source/platform/textdecoder.

Actually, that won't work because there's a dependency on HTMLTokenizer... 
Maybe we can move it into the parser directory?

[oystein (OOO til 10th of July), 2013-11-25 22:24:21]

On 2013/11/25 22:13:13, abarth wrote:
> On 2013/11/25 22:12:35, abarth wrote:
> > For example, we could move them to Source/platform/textdecoder.
> 
> Actually, that won't work because there's a dependency on HTMLTokenizer... 
> Maybe we can move it into the parser directory?

Yeah I think I'll do that. eseidel suggested as much earlier as well; it's
slightly off since it handles more than HTML, but it's probably the best fit
right now.

[eseidel, 2013-11-26 00:02:14]

I should have caught the thread-safey issue.  Thankfully we have abarth around
to keep us all honest. :)

[oystein (OOO til 10th of July), 2013-11-26 00:45:42]

On 2013/11/25 22:12:35, abarth wrote:
> On 2013/11/25 22:07:38, Oystein wrote:
> > I thought AtomicStrings used TLS for its string table, and the idea is that
> they
> > can be created and used on any thread, as long as instances don't cross any
> > thread boundaries?
> 
> That's correct, but we don't have an AtomicString table on the parser thread
and
> we don't want to create one.  We should probably have a DCHECK somewhere to
> prevent it from slipping in.

I looked into the HTMLIdentifier now, and a few questions present themselves.

* As it stands now, an HTMLIdentifier instance can only be created on the main
thread; it doesn't use TLS like AtomicString does for its string table, and will
assert if one is constructed on another thread.

* In this particular use case (HTMLMetaCharsetParser) we can't pre-create these
and pass them to the parser thread, so the HTMLIdentifier class would have to be
changed to use a thread-safe table allocation.

* If the table allocation is changed to keep tables in TLS, it would mean
HTMLIdentifier instances couldn't be passed between threads anymore, since they
wouldn't be comparable by pointer-value alone anymore due to having one table
per thread. This would break the existing usage of the HTMLIdentifier in the
CompactHTMLToken (and possibly elsewhere), which means we'd have to use some
locking mechanism to protect a global string table instead.... which isn't
particularly nice.

* It seems the main two advantages the HTMLIdentifier has over an AtomicString
is that it 1) Has a smaller table, since there's only a small set of static
strings in there, and 2) They can be passed between threads after creation.

If I'm correct in the above, I'm not sure what advantage we get for using
HTMLIdentifier in this case rather than an AtomicString. The cost of using the
latter in this case would be pretty much the same (since the string table for
the AtomicString in the parser thread would the same set of strings as the
HTMLIdentifier), and the instances in this case (HTMLMetaCharsetParser) aren't
passed between threads. On the other hand, the cost of using the former would
incur either some big rewrites or adding global locks.


Thoughts?

[oystein (OOO til 10th of July), 2013-11-26 01:00:43]

On 2013/11/26 00:45:42, Oystein wrote:
> On 2013/11/25 22:12:35, abarth wrote:
> > On 2013/11/25 22:07:38, Oystein wrote:
> > > I thought AtomicStrings used TLS for its string table, and the idea is
that
> > they
> > > can be created and used on any thread, as long as instances don't cross
any
> > > thread boundaries?
> > 
> > That's correct, but we don't have an AtomicString table on the parser thread
> and
> > we don't want to create one.  We should probably have a DCHECK somewhere to
> > prevent it from slipping in.
> 
> I looked into the HTMLIdentifier now, and a few questions present themselves.
> 
> * As it stands now, an HTMLIdentifier instance can only be created on the main
> thread; it doesn't use TLS like AtomicString does for its string table, and
will
> assert if one is constructed on another thread.
> 
> * In this particular use case (HTMLMetaCharsetParser) we can't pre-create
these
> and pass them to the parser thread, so the HTMLIdentifier class would have to
be
> changed to use a thread-safe table allocation.
> 
> * If the table allocation is changed to keep tables in TLS, it would mean
> HTMLIdentifier instances couldn't be passed between threads anymore, since
they
> wouldn't be comparable by pointer-value alone anymore due to having one table
> per thread. This would break the existing usage of the HTMLIdentifier in the
> CompactHTMLToken (and possibly elsewhere), which means we'd have to use some
> locking mechanism to protect a global string table instead.... which isn't
> particularly nice.
> 
> * It seems the main two advantages the HTMLIdentifier has over an AtomicString
> is that it 1) Has a smaller table, since there's only a small set of static
> strings in there, and 2) They can be passed between threads after creation.
> 
> If I'm correct in the above, I'm not sure what advantage we get for using
> HTMLIdentifier in this case rather than an AtomicString. The cost of using the
> latter in this case would be pretty much the same (since the string table for
> the AtomicString in the parser thread would the same set of strings as the
> HTMLIdentifier), and the instances in this case (HTMLMetaCharsetParser) aren't
> passed between threads. On the other hand, the cost of using the former would
> incur either some big rewrites or adding global locks.
> 
> 
> Thoughts?

...and as I hit Send it just occurred to me that the string table in
HTMLIdentifier is only ever modified on the main thread as the set of actual
strings is static, so we can actually just remove the ASSERT(isMainThread())
when all we need to do is a lookup (or move it to HTMLIdentifier::addNames).

[oystein (OOO til 10th of July), 2013-11-26 01:03:39]

On 2013/11/26 01:00:43, Oystein wrote:
> On 2013/11/26 00:45:42, Oystein wrote:
> > On 2013/11/25 22:12:35, abarth wrote:
> > > On 2013/11/25 22:07:38, Oystein wrote:
> > > > I thought AtomicStrings used TLS for its string table, and the idea is
> that
> > > they
> > > > can be created and used on any thread, as long as instances don't cross
> any
> > > > thread boundaries?
> > > 
> > > That's correct, but we don't have an AtomicString table on the parser
thread
> > and
> > > we don't want to create one.  We should probably have a DCHECK somewhere
to
> > > prevent it from slipping in.
> > 
> > I looked into the HTMLIdentifier now, and a few questions present
themselves.
> > 
> > * As it stands now, an HTMLIdentifier instance can only be created on the
main
> > thread; it doesn't use TLS like AtomicString does for its string table, and
> will
> > assert if one is constructed on another thread.
> > 
> > * In this particular use case (HTMLMetaCharsetParser) we can't pre-create
> these
> > and pass them to the parser thread, so the HTMLIdentifier class would have
to
> be
> > changed to use a thread-safe table allocation.
> > 
> > * If the table allocation is changed to keep tables in TLS, it would mean
> > HTMLIdentifier instances couldn't be passed between threads anymore, since
> they
> > wouldn't be comparable by pointer-value alone anymore due to having one
table
> > per thread. This would break the existing usage of the HTMLIdentifier in the
> > CompactHTMLToken (and possibly elsewhere), which means we'd have to use some
> > locking mechanism to protect a global string table instead.... which isn't
> > particularly nice.
> > 
> > * It seems the main two advantages the HTMLIdentifier has over an
AtomicString
> > is that it 1) Has a smaller table, since there's only a small set of static
> > strings in there, and 2) They can be passed between threads after creation.
> > 
> > If I'm correct in the above, I'm not sure what advantage we get for using
> > HTMLIdentifier in this case rather than an AtomicString. The cost of using
the
> > latter in this case would be pretty much the same (since the string table
for
> > the AtomicString in the parser thread would the same set of strings as the
> > HTMLIdentifier), and the instances in this case (HTMLMetaCharsetParser)
aren't
> > passed between threads. On the other hand, the cost of using the former
would
> > incur either some big rewrites or adding global locks.
> > 
> > 
> > Thoughts?
> 
> ...and as I hit Send it just occurred to me that the string table in
> HTMLIdentifier is only ever modified on the main thread as the set of actual
> strings is static, so we can actually just remove the ASSERT(isMainThread())
> when all we need to do is a lookup (or move it to HTMLIdentifier::addNames).

(In other words: Please ignore my above wall of text :) ).

[oystein (OOO til 10th of July), 2013-11-27 00:47:29]

>In the first case, we should break the dependency from HTMLMetaCharsetParser on
>AtomicString and use HTMLIdentifier instead.  For the second case, we should
>move DOMImplementation::isXMLMIMEType to some other file that contains
>cross-thread code.

I've replaced the AtomicString usage with HTMLIdentifier now; the only thing I'm
a bit iffy on is the change in PageSerializer.cpp; alternate solutions are
welcome.

For the DOMImplementation::isXMIMEType: Any suggestions for a better location?

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3972: m_visuallyOrdered = !m_visuallyOrdered;
On 2013/11/25 19:57:13, eseidel wrote:
> This reads weird to me, but OK.

Habitual thing, I suppose; I added a new local var instead.

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
File Source/core/dom/DocumentEncodingData.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/dom/Document...
Source/core/dom/DocumentEncodingData.cpp:31: 
On 2013/11/25 21:45:40, abarth wrote:
> You've got an extra blank line here.

Done.

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:981: memcpy(buffer->data(), data,
length);
On 2013/11/25 21:45:40, abarth wrote:
> It's too bad we end up with an extra memcpy here.  I guess this will go away
> once the network stack can give us the bytes directly on the background
thread.

Yep it'll go away then (or at least only be used for the first chunk and any
in-flight data coming in while we set up the rerouting filter). 

I think I mentioned it above, but we may be able to get rid of it if we change
the appendBytes() virtual to pass around a SharedBuffer rather than just a raw
pointer+length, but if that's decided on I think it should be a separate CL from
this.

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditor.cpp (right):

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:310: if (!m_httpBodyAsString.isEmpty()) {
On 2013/11/25 21:45:40, abarth wrote:
> Should we clear out  m_httpBodyAsString after this runs?

Actually I meant to ask about that; there's some logic in
Document::setEncodingData which handles a situation where the encoding changes
while we're processing <head>. If that can happen, we'd still need to keep
m_httpBodyAsString around in case setEncoding() gets called again.

https://codereview.chromium.org/74513003/diff/530001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:737: &&
m_decodedHTTPBody.isSafeToSendToAnotherThread();
On 2013/11/25 21:45:40, abarth wrote:
> Don't we need to add m_httpBodyAsString to this list?

Done.

[abarth-chromium, 2013-11-27 18:18:08]

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
File Source/core/html/parser/HTMLIdentifier.h (right):

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLIdentifier.h:59: HTMLIdentifier(const AtomicString&
str, CharacterWidth width)
str -> string

Please use complete works in variable names.

It seems very strange that we would want to take an AtomicString parameter here.
 Why not a String parameter?

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLIdentifier.h:60: :
m_string(findIfKnown(str.characters16(), str.length()))
How do you know it's safe to call characters16?  What if the string consists of
8 bit characters?

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLIdentifier.h:82: bool operator!=(const
QualifiedName& b) const;
Why does one of these functions have a formal parameter name while the other
does not?

I'd prefer we omitted these operators.  It's better for callers to be explicit
about what comparison operators they want to perform.

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTokenizer.h (right):

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLTokenizer.h:181: void updateStateFor(const
HTMLIdentifier& tagName);
Rather than introduce an AtomicString -> HTMLIdentifier conversion, I would have
just overloaded this function.

[oystein (OOO til 10th of July), 2013-11-27 18:46:32]

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
File Source/core/html/parser/HTMLIdentifier.h (right):

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLIdentifier.h:59: HTMLIdentifier(const AtomicString&
str, CharacterWidth width)
On 2013/11/27 18:18:08, abarth wrote:
> str -> string
> 
> Please use complete works in variable names.
> 
Done.

> It seems very strange that we would want to take an AtomicString parameter
here.
>  Why not a String parameter?

If I need to handle both the 8bit and the 16bit case, then yeah a String
parameter would make sense. If I was correct in the assumption below then maybe
it should be a QualifiedName parameter.

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLIdentifier.h:60: :
m_string(findIfKnown(str.characters16(), str.length()))
On 2013/11/27 18:18:08, abarth wrote:
> How do you know it's safe to call characters16?  What if the string consists
of
> 8 bit characters?

This is only ever used by the QualifiedName -> HTMLIdentifier conversion in
PageSerializer.cpp, I was assuming that'd always be unicode but if that's
incorrect I can add some handling of both (why do we have so many
almost-identical string classes, and keep having to convert between them?).

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLIdentifier.h:82: bool operator!=(const
QualifiedName& b) const;
On 2013/11/27 18:18:08, abarth wrote:
> Why does one of these functions have a formal parameter name while the other
> does not?
> 
> I'd prefer we omitted these operators.  It's better for callers to be explicit
> about what comparison operators they want to perform.

Hm what ambiguity is there when comparing a QualifiedName object and a
HTMLIdentifier object?

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTokenizer.h (right):

https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
Source/core/html/parser/HTMLTokenizer.h:181: void updateStateFor(const
HTMLIdentifier& tagName);
On 2013/11/27 18:18:08, abarth wrote:
> Rather than introduce an AtomicString -> HTMLIdentifier conversion, I would
have
> just overloaded this function.

Which conversion do you mean here? The only place I'm introducing a conversion
like that (I think?) is in the PageSerializer.cpp, and that doesn't call into
HTMLTokenizer, it calls into HTMLMetaCharsetParser.

[abarth-chromium, 2013-11-27 18:57:19]

On 2013/11/27 18:46:32, Oystein wrote:
> > It seems very strange that we would want to take an AtomicString parameter
> here.
> >  Why not a String parameter?
> 
> If I need to handle both the 8bit and the 16bit case, then yeah a String
> parameter would make sense. If I was correct in the assumption below then
maybe
> it should be a QualifiedName parameter.

Instead, I would recommend limiting the use of HTMLIdentifier to the parser
thread and avoiding the need to convert from a String to an HTMLIdentifier.

>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> Source/core/html/parser/HTMLIdentifier.h:60: :
> m_string(findIfKnown(str.characters16(), str.length()))
> On 2013/11/27 18:18:08, abarth wrote:
> > How do you know it's safe to call characters16?  What if the string consists
> of
> > 8 bit characters?
> 
> This is only ever used by the QualifiedName -> HTMLIdentifier conversion in
> PageSerializer.cpp, I was assuming that'd always be unicode but if that's
> incorrect I can add some handling of both (why do we have so many
> almost-identical string classes, and keep having to convert between them?).

They serve different purposes.  Here's a doc that contains some information
about them:

https://docs.google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLaOYRbiHz5TiGJl...

In particular, QualifiedName isn't a string.  It's a struct that contains three
strings.  HTMLIdentifier doesn't need to exist anymore, but we haven't gotten
around to removing it yet.

Given a string (of any time), you can't make assumptions about whether it
contains 8 bit or 16 bit characters.  They're two representations for the same
type.

>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> Source/core/html/parser/HTMLIdentifier.h:82: bool operator!=(const
> QualifiedName& b) const;
> On 2013/11/27 18:18:08, abarth wrote:
> > Why does one of these functions have a formal parameter name while the other
> > does not?
> > 
> > I'd prefer we omitted these operators.  It's better for callers to be
explicit
> > about what comparison operators they want to perform.
> 
> Hm what ambiguity is there when comparing a QualifiedName object and a
> HTMLIdentifier object?

Well, HTMLIdentifier represents a single string whereas QualifiedName represents
three strings.  There's no way for them to be equal.  In most cases, folks are
interested in matching HTMLIdentifiers against the localName of the
QualifiedName.  We have free functions that make that easy.  Overloading
operators makes the code less explicit, which makes it harder to see what's
going on.

>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLTokenizer.h (right):
> 
>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> Source/core/html/parser/HTMLTokenizer.h:181: void updateStateFor(const
> HTMLIdentifier& tagName);
> On 2013/11/27 18:18:08, abarth wrote:
> > Rather than introduce an AtomicString -> HTMLIdentifier conversion, I would
> have
> > just overloaded this function.
> 
> Which conversion do you mean here? The only place I'm introducing a conversion
> like that (I think?) is in the PageSerializer.cpp, and that doesn't call into
> HTMLTokenizer, it calls into HTMLMetaCharsetParser.

My point is that the API for HTMLIdentifier shouldn't mention AtomicString.  The
reason HTMLIdentifier exists is so that we can avoid using AtomicStrings on the
parser thread.  It's very strange to need to convert an AtomicString to an
HTMLIdentifier.  If you have an AtomicString, that implies you're not on the
parser thread, which implies you have no need for an HTMLIdentifier.

[oystein (OOO til 10th of July), 2013-11-27 19:17:04]

On 2013/11/27 18:57:19, abarth wrote:
> On 2013/11/27 18:46:32, Oystein wrote:
> > > It seems very strange that we would want to take an AtomicString parameter
> > here.
> > >  Why not a String parameter?
> > 
> > If I need to handle both the 8bit and the 16bit case, then yeah a String
> > parameter would make sense. If I was correct in the assumption below then
> maybe
> > it should be a QualifiedName parameter.
> 
> Instead, I would recommend limiting the use of HTMLIdentifier to the parser
> thread and avoiding the need to convert from a String to an HTMLIdentifier.

(See below)

> >
>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> > Source/core/html/parser/HTMLIdentifier.h:60: :
> > m_string(findIfKnown(str.characters16(), str.length()))
> > On 2013/11/27 18:18:08, abarth wrote:
> > > How do you know it's safe to call characters16?  What if the string
consists
> > of
> > > 8 bit characters?
> > 
> > This is only ever used by the QualifiedName -> HTMLIdentifier conversion in
> > PageSerializer.cpp, I was assuming that'd always be unicode but if that's
> > incorrect I can add some handling of both (why do we have so many
> > almost-identical string classes, and keep having to convert between them?).
> 
> They serve different purposes.  Here's a doc that contains some information
> about them:
> 
>
https://docs.google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLaOYRbiHz5TiGJl...
> 
> In particular, QualifiedName isn't a string.  It's a struct that contains
three
> strings.  HTMLIdentifier doesn't need to exist anymore, but we haven't gotten
> around to removing it yet.
> 
> Given a string (of any time), you can't make assumptions about whether it
> contains 8 bit or 16 bit characters.  They're two representations for the same
> type.

Ok; I'll update this pending the rest of the discussion.

> >
>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> > Source/core/html/parser/HTMLIdentifier.h:82: bool operator!=(const
> > QualifiedName& b) const;
> > On 2013/11/27 18:18:08, abarth wrote:
> > > Why does one of these functions have a formal parameter name while the
other
> > > does not?
> > > 
> > > I'd prefer we omitted these operators.  It's better for callers to be
> explicit
> > > about what comparison operators they want to perform.
> > 
> > Hm what ambiguity is there when comparing a QualifiedName object and a
> > HTMLIdentifier object?
> 
> Well, HTMLIdentifier represents a single string whereas QualifiedName
represents
> three strings.  There's no way for them to be equal.  In most cases, folks are
> interested in matching HTMLIdentifiers against the localName of the
> QualifiedName.  We have free functions that make that easy.  Overloading
> operators makes the code less explicit, which makes it harder to see what's
> going on.


Fair point, that makes sense; I didn't realize QualifiedName was a compound
string.

> >
>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> > File Source/core/html/parser/HTMLTokenizer.h (right):
> > 
> >
>
https://codereview.chromium.org/74513003/diff/610001/Source/core/html/parser/...
> > Source/core/html/parser/HTMLTokenizer.h:181: void updateStateFor(const
> > HTMLIdentifier& tagName);
> > On 2013/11/27 18:18:08, abarth wrote:
> > > Rather than introduce an AtomicString -> HTMLIdentifier conversion, I
would
> > have
> > > just overloaded this function.
> > 
> > Which conversion do you mean here? The only place I'm introducing a
conversion
> > like that (I think?) is in the PageSerializer.cpp, and that doesn't call
into
> > HTMLTokenizer, it calls into HTMLMetaCharsetParser.
> 
> My point is that the API for HTMLIdentifier shouldn't mention AtomicString. 
The
> reason HTMLIdentifier exists is so that we can avoid using AtomicStrings on
the
> parser thread.  It's very strange to need to convert an AtomicString to an
> HTMLIdentifier.  If you have an AtomicString, that implies you're not on the
> parser thread, which implies you have no need for an HTMLIdentifier.


All of that relies on the assumption that none of the code running on the parser
thread will be used on the main thread though. In this specific case
PageSerializer (running on the main thread) uses HTMLMetaCharsetParser, which is
also used by the parser. It seems more productive to ensure code which is shared
between threads remains threadsafe rather than trying to keep everything
separate and end up with a large amount of duplicate functionality. I'm also
still unclear on what benefits there are to restricting AtomicString usage to
the main thread which is worth all of this :/.

[abarth-chromium, 2013-12-02 19:23:35]

On 2013/11/27 19:17:04, Oystein wrote:
> All of that relies on the assumption that none of the code running on the
parser
> thread will be used on the main thread though. In this specific case
> PageSerializer (running on the main thread) uses HTMLMetaCharsetParser, which
is
> also used by the parser.

Hum...  PageSerializer only needs to call
HTMLMetaCharsetParser::encodingFromMetaAttributes, which is a static.  We
probably should just disentangle these two classes.

> It seems more productive to ensure code which is shared
> between threads remains threadsafe rather than trying to keep everything
> separate and end up with a large amount of duplicate functionality.

The approach we've taken so far is to make some layers of the system threadsafe
(e.g., TextEncoding) and to make other layers of the system thread-specific.  To
avoid duplicated functionality, we've used templates in a few places to share
logic between threads without needing to share the same types.

> I'm also
> still unclear on what benefits there are to restricting AtomicString usage to
> the main thread which is worth all of this :/.

There's no reason for the parser thread to have an AtomicStringTable because the
parser thread never retains strings.  Having an AtomicStringTable is either
wasteful or a threading problem.

These problems would be much easier if we had a thread-safe String class, but
currently we don't.  Without a thread-safe String class, it's difficult to make
code that interacts heavily with String thread-safe.  Instead, we've gone the
route of making that sort of code thread-specific and using types like
HTMLIdentifier to help us avoid making threading errors.  Now that we've gotten
better at threading (i.e., now that HTMLNames has thread-safe strings), we don't
need HTMLIdentifier anymore.  Maybe a good warmup CL would be to delete
HTMLIdentifier?

[oystein (OOO til 10th of July), 2013-12-04 18:48:18]

On 2013/12/02 19:23:35, abarth wrote:
> Hum...  PageSerializer only needs to call
> HTMLMetaCharsetParser::encodingFromMetaAttributes, which is a static.  We
> probably should just disentangle these two classes.
> 

Disentangle and template-ize on the attribute name, to handle either string
type? Any better location for it?

> The approach we've taken so far is to make some layers of the system
threadsafe
> (e.g., TextEncoding) and to make other layers of the system thread-specific. 
To
> avoid duplicated functionality, we've used templates in a few places to share
> logic between threads without needing to share the same types.

Gotcha, thanks for the background info. I'll save my thoughts on the long-term
viability of that for another time and place :).

> There's no reason for the parser thread to have an AtomicStringTable because
the
> parser thread never retains strings.  Having an AtomicStringTable is either
> wasteful or a threading problem.

Has there been any discussion about sharing the AtomicStringTable between
threads, and protecting it with a read/write lock? Depending on how often and in
what situations new strings are added to the table, there may actually be a very
low degree of contention here and may be a good solution.

> These problems would be much easier if we had a thread-safe String class, but
> currently we don't.  Without a thread-safe String class, it's difficult to
make
> code that interacts heavily with String thread-safe.  Instead, we've gone the
> route of making that sort of code thread-specific and using types like
> HTMLIdentifier to help us avoid making threading errors.  Now that we've
gotten
> better at threading (i.e., now that HTMLNames has thread-safe strings), we
don't
> need HTMLIdentifier anymore.  Maybe a good warmup CL would be to delete
> HTMLIdentifier?

Thread-safe strings in this context is static strings? What should
HTMLIdentifier be replaced with?

[abarth-chromium, 2013-12-05 04:22:30]

On 2013/12/04 18:48:18, Oystein wrote:
> On 2013/12/02 19:23:35, abarth wrote:
> > Hum...  PageSerializer only needs to call
> > HTMLMetaCharsetParser::encodingFromMetaAttributes, which is a static.  We
> > probably should just disentangle these two classes.
> 
> Disentangle and template-ize on the attribute name, to handle either string
> type? Any better location for it?

We can just move it to be a free function in some other file, like we have for
HTMLParserIdioms.

> > There's no reason for the parser thread to have an AtomicStringTable because
> the
> > parser thread never retains strings.  Having an AtomicStringTable is either
> > wasteful or a threading problem.
> 
> Has there been any discussion about sharing the AtomicStringTable between
> threads, and protecting it with a read/write lock?

The StringImpl objects stored in the table don't have thread-safe ref counts, so
you'd quickly crash.

> Depending on how often and in
> what situations new strings are added to the table, there may actually be a
very
> low degree of contention here and may be a good solution.

AtomicStringTable lets you re-use StringImpl objects, but StringImpl objects
cannot be referenced from multiple threads in the general case.

> Thread-safe strings in this context is static strings?

Yes.

> What should HTMLIdentifier be replaced with?

Just String.  The reason HTMLIdentifier existed was to let us atomize strings on
the background thread without actually manipulating the ref count on the strings
in HTMLNames.  Now that HTMLNames has thread-safe strings HTMLIdentifier has no
reason for existing.

[eseidel, 2013-12-05 04:31:17]

StringImpl's marked with isStatic() don't have a thread-safe refcount, they just
ignore any attempt to destroy them when their refcount might happen to hit 0:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[oystein (OOO til 10th of July), 2013-12-10 00:12:44]

On 2013/12/05 04:22:30, abarth wrote:
> > What should HTMLIdentifier be replaced with?
> 
> Just String.  The reason HTMLIdentifier existed was to let us atomize strings
on
> the background thread without actually manipulating the ref count on the
strings
> in HTMLNames.  Now that HTMLNames has thread-safe strings HTMLIdentifier has
no
> reason for existing.

Gotcha, looking at this now. Should the HTMLIdentifier live on as essentially a
database of these static strings then, i.e. it'd have a "String
HTMLIdentifier::create(const Vector<UChar, inlineCapacity>& vector,
CharacterWidth width)", or do we have another way of finding a matching string
from HTMLNames when creating CompactHTMLTokens?

[abarth-chromium, 2013-12-10 01:14:12]

On 2013/12/10 00:12:44, Oystein wrote:
> Gotcha, looking at this now. Should the HTMLIdentifier live on as essentially
a
> database of these static strings then, i.e. it'd have a "String
> HTMLIdentifier::create(const Vector<UChar, inlineCapacity>& vector,
> CharacterWidth width)", or do we have another way of finding a matching string
> from HTMLNames when creating CompactHTMLTokens?

Yeah, I think we want some mechanism for doing that.  There's a Vector of all
the static StringImpls.  We should make that into a HashSet and add a way to
query it.  That can be generic at the WTF layer and doesn't need to be tied to
HTML.

[oystein (OOO til 10th of July), 2013-12-13 18:38:41]

To pick this up again: After the HTMLIdentifier removal landed, I replaced its
usage in HTMLMetaCharsetParser with String, and moved the static function which
got called from multiple threads to HTMLParserIdioms.

Not sure if there's anything else I'm missing?

[abarth-chromium, 2013-12-15 05:49:59]

LGTM

This version looks much better.  Thanks for cleaning up the HTMLIdentifier mess.
 I'm not 100% sure that this CL is exactly correct, but we can land it and see
what signals we get back.  Below are some minor comments.  Thanks again for the
CL.

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
File Source/core/html/parser/BackgroundHTMLParser.h (right):

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.h:68: void append(const String&);
Can we make this function private now?

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
File Source/core/html/parser/TextDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
Source/core/html/parser/TextDocumentParser.cpp:50: }
We should be able to remove this override now, right?

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditor.cpp (right):

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:314: }
Can you clear m_httpBodyAsString once we're done with it here?  We don't want to
retain it.

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditor.h (right):

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.h:120: String m_httpBodyAsString;
This string can be pretty large..

[oystein (OOO til 10th of July), 2013-12-16 19:33:28]

Great, thanks for the review :).

I already have something WIP ready for the next step of this that I'll put up
during the day at some point.

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
File Source/core/html/parser/BackgroundHTMLParser.h (right):

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
Source/core/html/parser/BackgroundHTMLParser.h:68: void append(const String&);
On 2013/12/15 05:50:00, abarth wrote:
> Can we make this function private now?

Done.

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
File Source/core/html/parser/TextDocumentParser.cpp (right):

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
Source/core/html/parser/TextDocumentParser.cpp:50: }
On 2013/12/15 05:50:00, abarth wrote:
> We should be able to remove this override now, right?

Done.

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditor.cpp (right):

https://codereview.chromium.org/74513003/diff/720001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:314: }
On 2013/12/15 05:50:00, abarth wrote:
> Can you clear m_httpBodyAsString once we're done with it here?  We don't want
to
> retain it.

Done.

[commit-bot: I haz the power, 2013-12-16 19:34:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/74513003/750001

[commit-bot: I haz the power, 2013-12-16 21:44:07]

Change committed as 163977

[oystein (OOO til 10th of July), 2013-12-18 20:02:59]

A revert of this CL has been created in
https://codereview.chromium.org/118573002/ by oysteine@chromium.org.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=329603.