Bug 47560 - Ignore document.write() when it comes from a network task
Summary: Ignore document.write() when it comes from a network task
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 41115
  Show dependency treegraph
 
Reported: 2010-10-12 16:06 PDT by Tony Gentilcore
Modified: 2010-10-21 18:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.08 KB, patch)
2010-10-19 16:43 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2010-10-20 16:36 PDT, Tony Gentilcore
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Add test. Factor new class out to new file. (23.85 KB, patch)
2010-10-21 09:55 PDT, Tony Gentilcore
abarth: review-
Details | Formatted Diff | Diff
Fix src removal bug (25.23 KB, patch)
2010-10-21 16:36 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2010-10-12 16:06:50 PDT
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
Comment 1 Tony Gentilcore 2010-10-19 16:43:01 PDT
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.
Comment 2 Tony Gentilcore 2010-10-19 16:53:05 PDT
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.
Comment 3 Adam Barth 2010-10-19 18:09:22 PDT
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.
Comment 4 Adam Barth 2010-10-19 18:09:50 PDT
(Sorry for the shallow review, just catching up during a meeting.)
Comment 5 Tony Gentilcore 2010-10-20 16:36:10 PDT
Created attachment 71359 [details]
Patch
Comment 6 Tony Gentilcore 2010-10-20 16:36:38 PDT
(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.
Comment 7 Adam Barth 2010-10-20 18:54:55 PDT
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?
Comment 8 Tony Gentilcore 2010-10-21 09:55:58 PDT
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?
Comment 9 Adam Barth 2010-10-21 11:45:01 PDT
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?
Comment 10 Adam Barth 2010-10-21 11:45:27 PDT
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.
Comment 11 Tony Gentilcore 2010-10-21 16:36:15 PDT
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.
Comment 12 Adam Barth 2010-10-21 17:41:50 PDT
Comment on attachment 71508 [details]
Fix src removal bug

Thanks Tony.
Comment 13 WebKit Commit Bot 2010-10-21 18:27:49 PDT
Comment on attachment 71508 [details]
Fix src removal bug

Clearing flags on attachment: 71508

Committed r70282: <http://trac.webkit.org/changeset/70282>
Comment 14 WebKit Commit Bot 2010-10-21 18:27:55 PDT
All reviewed patches have been landed.  Closing bug.