Bug 110276 – Fix crash in preloading scanning base tags with no href attribute for background parser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Fix crash in preloading scanning base tags with no href attribute for background parser

[Tony Gentilcore]

Created attachment 189193 [details]
Patch

[Eric Seidel (no email)]

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. :(

[Adam Barth]

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.

[Adam Barth]

s/save/safe/

[Tony Gentilcore]

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.

[Tony Gentilcore]

(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.

[Eric Seidel (no email)]

(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.

[Adam Barth]

Ah!  Ignore me.  :)

[Tony Gentilcore]

(In reply to comment #8)
> Ah!  Ignore me.  :)

Do you want to review now?

[Eric Seidel (no email)]

Comment on attachment 189193 [details]
Patch

By the power of justice.

[WebKit Review Bot]

Comment on attachment 189193 [details]
Patch

Clearing flags on attachment: 189193

Committed r143415: <http://trac.webkit.org/changeset/143415>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.