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

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.