Make BackgroundHTMLParser work with doc.writes that enter or leave foreign content

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tony Gentilcore]

Currently BackgroundHTMLParser keeps track of whether we are in foreign content mode. However, if a doc.write causes us to enter or leave foreign content mode, we'll miss it because doc.write is processed on the main thread.

Adam suggests we can fix this by reading the information from the stack of open elements and sending it to the background HTML parser in the Checkpoint sent in resumeFrom().

[Adam Barth]

This sounds important to fix.

[Adam Barth]

Created attachment 192563 [details]
work in progress

[Adam Barth]

Created attachment 192576 [details]
Patch

[Adam Barth]

Comment on attachment 192576 [details]
Patch

Tweaks coming.

[Adam Barth]

Created attachment 192578 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 192578 [details]
Patch

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

LGTM.  Please return the "simulator" prefix. :)

> Source/WebCore/html/parser/BackgroundHTMLParser.cpp:155
> +        if (!m_treeBuilder.simulate(m_pendingTokens->last(), m_tokenizer.get()) || m_pendingTokens->size() >= pendingTokenLimit)

This is a little confusing.  It's not really a tree-builder, and if it was you wouldn't tell it to "simulate". :)

> Source/WebCore/html/parser/HTMLTreeBuilder.h:71
> +    HTMLElementStack* openElements() const { return m_tree.openElements(); }

Should this be const?  We don't want folks modifying this. :)

> Source/WebCore/html/parser/HTMLTreeBuilderSimulator.cpp:136
> +    return namespaceStack;

You're going to end up copying the vector a couple times here.

[Adam Barth]

(In reply to comment #6)
> (From update of attachment 192578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192578&action=review
> 
> LGTM.  Please return the "simulator" prefix. :)

Done.

> > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:155
> > +        if (!m_treeBuilder.simulate(m_pendingTokens->last(), m_tokenizer.get()) || m_pendingTokens->size() >= pendingTokenLimit)
> 
> This is a little confusing.  It's not really a tree-builder, and if it was you wouldn't tell it to "simulate". :)

Fixed.

> > Source/WebCore/html/parser/HTMLTreeBuilder.h:71
> > +    HTMLElementStack* openElements() const { return m_tree.openElements(); }
> 
> Should this be const?  We don't want folks modifying this. :)

Fixed.

> > Source/WebCore/html/parser/HTMLTreeBuilderSimulator.cpp:136
> > +    return namespaceStack;
> 
> You're going to end up copying the vector a couple times here.

Yeah, I'd be surprised if that was a big problem.  We only call this function after executing script.  I'm inclined to iterate in a future patch if we see this on a profile.

[Adam Barth]

Created attachment 192583 [details]
Patch for landing

[Adam Barth]

Created attachment 192598 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 192598 [details]
Patch for landing

Clearing flags on attachment: 192598

Committed r145464: <http://trac.webkit.org/changeset/145464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.