StartTagScanner should be thread-safe

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

StartTagScanner should be thread-safe

[Adam Barth]

Created attachment 188186 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 188186 [details]
Patch

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

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:109
> +        if (m_tagId == UnknownTagId)
>              return;

I think this is very clever.  It's not clear to me that the HTMLTagIdentifier stuff needs to leak out of this class at all.  It could just be private to StartTagScanner?  I assume it's part of a larger plan you have for world domination, hence being separate?

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:112
>              AtomicString attributeName(iter->name);

I believe this will crash on the background thread, no?  It looks like you don't even use it as Atomic, but making it Atomic does prevent a copy in teh common case on the main thread.

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 188186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188186&action=review
> 
> > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:109
> > +        if (m_tagId == UnknownTagId)
> >              return;
> 
> I think this is very clever.  It's not clear to me that the HTMLTagIdentifier stuff needs to leak out of this class at all.  It could just be private to StartTagScanner?  I assume it's part of a larger plan you have for world domination, hence being separate?

It's going to leak out of the class, but hopefully not out of the file.  For example, I plan to use it in processPossibleStyleTag as well.

> > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:112
> >              AtomicString attributeName(iter->name);
> 
> I believe this will crash on the background thread, no?  It looks like you don't even use it as Atomic, but making it Atomic does prevent a copy in teh common case on the main thread.

Yeah, processAttributes(const HTMLToken::AttributeList& attributes) is going to be main thread only.  I plan to add a processAttributes that takes a CompactHTMLToken::AttributeList or whatever the actual type is called.  I can add an ASSERT(isMainThread()) to this function if that would be helpful.

[Adam Barth]

Review ping.  This patch is blocking this stack of patches.  :)

[Eric Seidel (no email)]

Comment on attachment 188186 [details]
Patch

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

LGTM.

>>> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:112
>>>              AtomicString attributeName(iter->name);
>> 
>> I believe this will crash on the background thread, no?  It looks like you don't even use it as Atomic, but making it Atomic does prevent a copy in teh common case on the main thread.
> 
> Yeah, processAttributes(const HTMLToken::AttributeList& attributes) is going to be main thread only.  I plan to add a processAttributes that takes a CompactHTMLToken::AttributeList or whatever the actual type is called.  I can add an ASSERT(isMainThread()) to this function if that would be helpful.

I think ASSERTing when we should be on the main thread is helpful, yes.

[Adam Barth]

Created attachment 188243 [details]
Patch for landing

[Adam Barth]

Comment on attachment 188243 [details]
Patch for landing

Clearing flags on attachment: 188243

Committed r142840: <http://trac.webkit.org/changeset/142840>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.