Summary: | Fix crash in preloading scanning base tags with no href attribute for background parser | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||
Component: | New Bugs | Assignee: | 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
Tony Gentilcore
2013-02-19 16:02:10 PST
Created attachment 189193 [details]
Patch
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 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. s/save/safe/ 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. (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. (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. Ah! Ignore me. :) (In reply to comment #8) > Ah! Ignore me. :) Do you want to review now? Comment on attachment 189193 [details]
Patch
By the power of justice.
Comment on attachment 189193 [details] Patch Clearing flags on attachment: 189193 Committed r143415: <http://trac.webkit.org/changeset/143415> All reviewed patches have been landed. Closing bug. |