Bug 110276 - Fix crash in preloading scanning base tags with no href attribute for background parser
Summary: Fix crash in preloading scanning base tags with no href attribute for backgro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 106127
  Show dependency tree / graph
 
Reported: 2013-02-19 16:02 PST by Tony Gentilcore
Modified: 2013-02-19 17:22 PST (History)
4 users (show)

See Also:

Attachments
Patch (13.93 KB, patch)
2013-02-19 16:06 PST, Tony Gentilcore 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.