Threaded HTML parser fails resources/plain-text-unsafe.dat

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Barth]

resources/plain-text-unsafe.dat:
15
16
17
18
21

Looks like a problem with replacing null characters in foreign content.

It's a slight pain to run these tests in the threaded parser at the moment.

[Tony Gentilcore]

I've been wondering why we weren't seeing any failures due to the missing setForceNullCharacterReplacement() call. If you clue me in as to the easiest way to run these tests with the threaded parser, I'm happy to fix this tomorrow.

[Adam Barth]

Created attachment 189652 [details]
Hacks to be able to run the html5lib tests

On my machine, runner.html times out, so you also need to break it down into smaller pieces.  (The document.write path is faster than the data URL path.)

[Tony Gentilcore]

Created attachment 189858 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 189858 [details]
Patch

LGTM.  Don't we also need to do null character replacement in textArea and a couple others?

[Eric Seidel (no email)]

Comment on attachment 189858 [details]
Patch

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

> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:117
> +    const String& tagName = token.data();

Is there not a name() accessor?

[Adam Barth]

Comment on attachment 189858 [details]
Patch

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

> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:115
> +static inline bool tokenExitsSVG(const CompactHTMLToken& token)

Darin would tell us that static and inline are redundant here.  :)

[Eric Seidel (no email)]

Comment on attachment 189858 [details]
Patch

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

>> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:115
>> +static inline bool tokenExitsSVG(const CompactHTMLToken& token)
> 
> Darin would tell us that static and inline are redundant here.  :)

Really?

[Tony Gentilcore]

(In reply to comment #7)
> (From update of attachment 189858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189858&action=review
> 
> >> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:115
> >> +static inline bool tokenExitsSVG(const CompactHTMLToken& token)
> > 
> > Darin would tell us that static and inline are redundant here.  :)
> 
> Really?

Surprising to me too. I'm interested in the explanation.

> Don't we also need to do null character replacement in textArea and a couple others?

Yeah, those tags kick us into TextMode, hence the remaining FIXME.

> Is there not a name() accessor?

No, these are CompactHTMLTokens, everything is in data(). The other accessors would just be an assertion on the correct type and a return of m_data.

[Adam Barth]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 189858 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189858&action=review
> > 
> > >> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:115
> > >> +static inline bool tokenExitsSVG(const CompactHTMLToken& token)
> > > 
> > > Darin would tell us that static and inline are redundant here.  :)
> > 
> > Really?
> 
> Surprising to me too. I'm interested in the explanation.

Yeah, they both just mean "give this function internal linkage" in this context.

[Eric Seidel (no email)]

Comment on attachment 189858 [details]
Patch

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

>>>>> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:115
>>>>> +static inline bool tokenExitsSVG(const CompactHTMLToken& token)
>>>> 
>>>> Darin would tell us that static and inline are redundant here.  :)
>>> 
>>> Really?
>> 
>> Surprising to me too. I'm interested in the explanation.
> 
> Yeah, they both just mean "give this function internal linkage" in this context.

So what you're saying is that "inline" is meaningless?  (outside of aliasing to static for a free function in a .cpp file?)

[Adam Barth]

(In reply to comment #10)
> (From update of attachment 189858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189858&action=review
> 
> >>>>> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:115
> >>>>> +static inline bool tokenExitsSVG(const CompactHTMLToken& token)
> >>>> 
> >>>> Darin would tell us that static and inline are redundant here.  :)
> >>> 
> >>> Really?
> >> 
> >> Surprising to me too. I'm interested in the explanation.
> > 
> > Yeah, they both just mean "give this function internal linkage" in this context.
> 
> So what you're saying is that "inline" is meaningless?  (outside of aliasing to static for a free function in a .cpp file?)

Not always.  Consider the following situation:

class A {
  XXX void f();
};

void A::f() { }

We can't use "static" for XXX because that would mean f wouldn't get a |this| parameter.  We can, however, use "inline" to give A::f internal linkage.

[Tony Gentilcore]

Created attachment 189873 [details]
Patch for landing

[Tony Gentilcore]

Interesting. It makes sense now that I think about it. Anyway, I killed the "inline" in front of those methods.

[WebKit Review Bot]

Comment on attachment 189873 [details]
Patch for landing

Clearing flags on attachment: 189873

Committed r143830: <http://trac.webkit.org/changeset/143830>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.