Bug 67214 – PreloadScanner shouldn't load images inside noscript via doc.write

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

[Tony Gentilcore]

PreloadScanner shouldn't load images inside noscript via doc.write

[Tony Gentilcore]

Created attachment 105662 [details]
Patch

[Darin Adler]

Comment on attachment 105662 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.h:154
> +    // Check the document.write() output with a separate preload scanner as
> +    // the main scanner can't deal with insertions.

I liked this comment better in its old location.

[Tony Gentilcore]

Apparently this bug hit a lot of DoubleClick users:
http://code.google.com/p/chromium/issues/detail?id=86911

I'm not thrilled about this approach, but it seems workable. The problem mentioned in the ChangeLog could be mitigated by clearing the m_insertionPreloadScanner after each script block is executed. But I'm not sure whether that is even worthwhile. PTAL and let me know if you have any other ideas.

[Tony Gentilcore]

(In reply to comment #2)
> (From update of attachment 105662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105662&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.h:154
> > +    // Check the document.write() output with a separate preload scanner as
> > +    // the main scanner can't deal with insertions.
> 
> I liked this comment better in its old location.

OK, I'll move the comment back before landing.

But I'll wait for feedback from Antti/Adam on the overall approach before uploading a new version of the patch.

[Adam Barth]

Comment on attachment 105662 [details]
Patch

Interesting.  Maybe we should clear the m_insertionPreloadScanner when we pop the current insertion point?  That would keep the tokenizer in a consistent state throughout a single insertion point but get a fresh state for each new insertion point.

[Adam Barth]

Comment on attachment 105662 [details]
Patch

This approach is also fine if there are some complexities in figuring out when it is safe to reset the state.

[Tony Gentilcore]

Created attachment 105679 [details]
Patch

[Tony Gentilcore]

Okay, now I'm clearing the insertion scanner in resumeParsingAfterScriptExecution() and I updated the test to make sure we don't get stuck in the noscript state.

I've also moved the comment back to the cpp file like Darin suggested.

Please take another look.

[WebKit Review Bot]

Comment on attachment 105679 [details]
Patch

Clearing flags on attachment: 105679

Committed r94111: <http://trac.webkit.org/changeset/94111>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.