Bug 47560 – Ignore document.write() when it comes from a network task

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Gentilcore]

The recommendation for setting the ignore-destructive-writes flag has just been updated:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=9767

We should update HTMLScriptRunner to match.

I believe this will fix: http://code.google.com/p/chromium/issues/detail?id=57196

[Tony Gentilcore]

Created attachment 71224 [details]
Patch

I've re-implemented NestingLevelIncrementer in Document, but would like to reuse the one in html/parser. If this patch is good, I can move NestingLevelIncrementer somewhere where it can be shared either right before or right after this patch.

[Tony Gentilcore]

A couple of notes:

 - This mitigates http://code.google.com/p/chromium/issues/detail?id=57196. With this patch, instead of showing a blank page, just the GlobalSign seal doesn't work (the seal graphic displays a message that says JS is disabled instead of saying the cert is valid). This is because the write() that injects the JS for the seal is now neutralized instead of letting it blow away the page. The app still needs to be updated to understand that WebKit supports defer now.
 - Due to https://bugs.webkit.org/show_bug.cgi?id=47940, I couldn't use the typical webkit-patch upload. So I manually did git format-patch and uploaded. It used a format that confuses the review tool and adds some stray characters to the end. I'll make sure this is resolved before landing.

[Adam Barth]

Comment on attachment 71224 [details]
Patch

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

> WebCore/dom/Document.h:1223
> -    // http://www.whatwg.org/specs/web-apps/current-work/#write-neutralised
> -    bool m_writeDisabled;
> +    // http://dev.w3.org/html5/spec/Overview.html#ignore-destructive-writes-counter

We usually reference www.whatwg.org for whatever reason.  I don't think the document will be on dev.w3.org in the long term.  In the long term the W3C docs move the to TR namespace.

> WebCore/dom/ScriptElement.cpp:198
> +            OwnPtr<DestructiveWriteCountIncrementer> ignoreDesctructiveWriteCountIncrementer;
> +            if (!m_scriptElement->sourceAttributeValue().isEmpty())
> +                ignoreDesctructiveWriteCountIncrementer = document->ignoreDestructiveWriteCountIncrementer();

Do we need the malloc here?  That seems unfortunately.  You can look at UserGestureIndicator to see one way to avoid this.

[Adam Barth]

(Sorry for the shallow review, just catching up during a meeting.)

[Tony Gentilcore]

Created attachment 71359 [details]
Patch

[Tony Gentilcore]

(In reply to comment #3)

> We usually reference www.whatwg.org for whatever reason.  I don't think the document will be on dev.w3.org in the long term.  In the long term the W3C docs move the to TR namespace.

Done.

> Do we need the malloc here?  That seems unfortunately.  You can look at UserGestureIndicator to see one way to avoid this.

Done. I temporarily added IgnoreDestructiveWriteCountIncrementer to Document.h. If everything looks good to you, I'll move it out to a new file.

[Adam Barth]

Comment on attachment 71359 [details]
Patch

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

> WebCore/dom/Document.h:1349
> +    IgnoreDestructiveWriteCountIncrementer()

Did you mean for this to be a destructor?  Can we add a test that observes the difference?

[Tony Gentilcore]

Created attachment 71447 [details]
Add test. Factor new class out to new file.

Oops. Good catch. I added a test that fails w/o the dtor and factored IgnoreDestructiveWriteCountIncrementer out to a new file.

Want to take another quick look?

[Adam Barth]

Comment on attachment 71447 [details]
Add test. Factor new class out to new file.

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

> WebCore/dom/ScriptElement.cpp:197
> +            bool external = !m_scriptElement->sourceAttributeValue().isEmpty();

Why can't an inline script have a sourceAttributeValue ?  I guess that would cause the script to be external.  What happens if we have a defer script that kicks off a load and then an inline script later comes and removes the src attribute (before the script executes)?  Will the script no longer be write disabled?

[Adam Barth]

Comment on attachment 71447 [details]
Add test. Factor new class out to new file.

Regardless of the answer to that question, we'd like a test for that case.

[Tony Gentilcore]

Created attachment 71508 [details]
Fix src removal bug

> Why can't an inline script have a sourceAttributeValue ?  I guess that would cause the script to be external.

From the spec: "For the purposes of these steps, the script is considered to be from an external file if, while the running a script algorithm above was running for this script, the script element had a src attribute specified."

> What happens if we have a defer script that kicks off a load and then an inline script later comes and removes the src attribute (before the script executes)?  Will the script no longer be write disabled?

Good catch! The spec does say "had a src attribute" not "has." I fixed that up and added a test case that failed previously.

[Adam Barth]

Comment on attachment 71508 [details]
Fix src removal bug

Thanks Tony.

[WebKit Commit Bot]

Comment on attachment 71508 [details]
Fix src removal bug

Clearing flags on attachment: 71508

Committed r70282: <http://trac.webkit.org/changeset/70282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.