Bug 47560

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.

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 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.

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

Created attachment 71359 [details] Patch

(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 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?

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 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 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.

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 on attachment 71508 [details] Fix src removal bug Thanks Tony.

Comment on attachment 71508 [details] Fix src removal bug Clearing flags on attachment: 71508 Committed r70282: <http://trac.webkit.org/changeset/70282>

All reviewed patches have been landed. Closing bug.