Bug 110276

Summary: Fix crash in preloading scanning base tags with no href attribute for background parser
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106127    
Attachments:
Description Flags
Patch none

Description Tony Gentilcore 2013-02-19 16:02:10 PST
Fix crash in preloading scanning base tags with no href attribute for background parser
Comment 1 Tony Gentilcore 2013-02-19 16:06:42 PST
Created attachment 189193 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-02-19 16:12:01 PST
Comment on attachment 189193 [details]
Patch

I was afraid of this.  I think this is a good change, but I don't really like how we're tying HTMLToken and CompactToken to the same API via the PreloadScanner template. :(
Comment 3 Adam Barth 2013-02-19 16:12:39 PST
Comment on attachment 189193 [details]
Patch

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

> Source/WebCore/html/parser/CompactHTMLToken.h:86
> +    Vector<Attribute> m_attributes;

Attribute contains AtomicString and QualifiedName, which means it's not save to move between threads.
Comment 4 Adam Barth 2013-02-19 16:13:02 PST
s/save/safe/
Comment 5 Tony Gentilcore 2013-02-19 16:16:58 PST
Comment on attachment 189193 [details]
Patch

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

>> Source/WebCore/html/parser/CompactHTMLToken.h:86
>> +    Vector<Attribute> m_attributes;
> 
> Attribute contains AtomicString and QualifiedName, which means it's not save to move between threads.

This is CompactHTMLToken::Attribute which contains only Strings. I can change all occurrences of Attribute to CompactHTMLToken::Attribute to be more explicit to make it more readable.
Comment 6 Tony Gentilcore 2013-02-19 16:18:40 PST
(In reply to comment #2)
> (From update of attachment 189193 [details])
> I was afraid of this.  I think this is a good change, but I don't really like how we're tying HTMLToken and CompactToken to the same API via the PreloadScanner template. :(

Would you prefer I undo it? I thought it was worth having the same API in order to avoid the code duplication. It's also possible a more similar API enhances readability.
Comment 7 Eric Seidel (no email) 2013-02-19 16:29:11 PST
(In reply to comment #6)
> (In reply to comment #2)
> > (From update of attachment 189193 [details] [details])
> > I was afraid of this.  I think this is a good change, but I don't really like how we're tying HTMLToken and CompactToken to the same API via the PreloadScanner template. :(
> 
> Would you prefer I undo it? I thought it was worth having the same API in order to avoid the code duplication. It's also possible a more similar API enhances readability.

No, this is the right change.  I just don't like the idea that we're limited to HTMLToken's API in CompactToken::Attribute.  But when we run up against those limits, then we can re-visit.
Comment 8 Adam Barth 2013-02-19 16:35:34 PST
Ah!  Ignore me.  :)
Comment 9 Tony Gentilcore 2013-02-19 16:37:39 PST
(In reply to comment #8)
> Ah!  Ignore me.  :)

Do you want to review now?
Comment 10 Eric Seidel (no email) 2013-02-19 16:42:02 PST
Comment on attachment 189193 [details]
Patch

By the power of justice.
Comment 11 WebKit Review Bot 2013-02-19 17:22:30 PST
Comment on attachment 189193 [details]
Patch

Clearing flags on attachment: 189193

Committed r143415: <http://trac.webkit.org/changeset/143415>
Comment 12 WebKit Review Bot 2013-02-19 17:22:34 PST
All reviewed patches have been landed.  Closing bug.