(Original report at http://crbug.com/78246) XMLTreeViewer (and associated XMLViewer.js) breaks in interesting and different ways when Javascript is disabled. In V8, none of XMLViewer.js runs, and we therefore crash at http://trac.webkit.org/browser/trunk/Source/WebCore/xml/XMLTreeViewer.cpp#L80 because the node wasn't added to the DOM by the script that didn't run. In JSC, we appear to run the initial script but block events, and the load event is required to display the document properly. abarth and I discussed this, and we think it might make sense to have some special rules for documents that are conceptually viewing source (HTMLViewSourceDocuments + documents using XMLTreeViewer). These documents would be permitted to run scripts regardless of user settings, but would be placed in a unique SecurityOrigin. I'll post a patch demonstrating the idea in a few minutes. Thoughts/concerns?
Created attachment 90553 [details] Unique SecurityOrigins for view source documents
Comment on attachment 90553 [details] Unique SecurityOrigins for view source documents View in context: https://bugs.webkit.org/attachment.cgi?id=90553&action=review > Source/WebCore/xml/XMLTreeViewer.cpp:74 > + m_document->setSecurityOrigin(uniqueOrigin.get()); This isn't right. You should be using setForcedSandboxFlags on FrameLoader before creating the document.
Created attachment 90760 [details] Temporary fix This is a patch to ensure that we displaying something of the xml document when script is disabled. We lose formatting and we strip the xml tags, but it's better than crashing or only showing boilerplate. I'll keep working on this, but I wanted to get something in before I lose the first half of next week to the contributor meeting :)
Comment on attachment 90760 [details] Temporary fix View in context: https://bugs.webkit.org/attachment.cgi?id=90760&action=review > Source/WebCore/xml/XMLTreeViewer.cpp:75 > + if (!m_document->frame()->script()->canExecuteScripts(NotAboutToExecuteScript)) Can frame() be null here?
(In reply to comment #4) > (From update of attachment 90760 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90760&action=review > > > Source/WebCore/xml/XMLTreeViewer.cpp:75 > > + if (!m_document->frame()->script()->canExecuteScripts(NotAboutToExecuteScript)) > > Can frame() be null here? This appears to only be reachable from Document::finishParsing(), which is only called from places where there's a valid Frame, and I don't see anywhere that could blow away the Frame after finishParsing() is entered. So I think the answer is "No", but I'm also confident that my manual code inspection doesn't constitute a proof :)
Comment on attachment 90760 [details] Temporary fix Clearing flags on attachment: 90760 Committed r84732: <http://trac.webkit.org/changeset/84732>
All reviewed patches have been landed. Closing bug.
Reopening, since the real fix is still in progress.
Created attachment 91361 [details] Attempt #2 at unique SecurityOrigins
This is definitely a patch for abarth.
Comment on attachment 91361 [details] Attempt #2 at unique SecurityOrigins View in context: https://bugs.webkit.org/attachment.cgi?id=91361&action=review This looks great. I would like to see one more round though because this stuff is delicate. > Source/WebCore/dom/Document.cpp:1669 > + m_frame->loader()->setForcedSandboxFlags(SandboxOrigin); Is this part necessary? If you're going to set the security origin forcibly on the next line, you probably don't need this line. > Source/WebCore/dom/Document.cpp:1742 > + if (m_isViewSource) > + m_frame->loader()->setForcedSandboxFlags(SandboxNone); This seems wrong. You might clobber other forced sandbox flags (e.g., in SVGImage). > Source/WebCore/bindings/ScriptControllerBase.cpp:42 > + if (m_frame->document() && m_frame->document()->isViewSource()) { Normally m_frame->document() is always non-null, but I think you're right that it can be null here. > Source/WebCore/loader/FrameLoader.h:255 > - void setForcedSandboxFlags(SandboxFlags flags) { m_forcedSandboxFlags = flags; m_sandboxFlags |= flags; } > + void setForcedSandboxFlags(SandboxFlags flags) { m_forcedSandboxFlags = flags; updateSandboxFlags(); } This change probably isn't necessary if we don't call setForcedSandboxFlags above.
Apparently I wrote that review a while ago and never clicked publish. Thankfully Ojan's auto-saving kept it for me!
> > Source/WebCore/dom/Document.cpp:1669 > > + m_frame->loader()->setForcedSandboxFlags(SandboxOrigin); > > Is this part necessary? If you're going to set the security origin forcibly on the next line, you probably don't need this line. The next line only sets the flags on the Document, not on the FrameLoader. > > > Source/WebCore/dom/Document.cpp:1742 > > + if (m_isViewSource) > > + m_frame->loader()->setForcedSandboxFlags(SandboxNone); > > This seems wrong. You might clobber other forced sandbox flags (e.g., in SVGImage). Hmm....I think it might be a moot point (the document will be going away soon since we're in detach(), right?), but yeah, I guess it does make sense to not try to remove the flags. > > Source/WebCore/loader/FrameLoader.h:255 > > - void setForcedSandboxFlags(SandboxFlags flags) { m_forcedSandboxFlags = flags; m_sandboxFlags |= flags; } > > + void setForcedSandboxFlags(SandboxFlags flags) { m_forcedSandboxFlags = flags; updateSandboxFlags(); } > > This change probably isn't necessary if we don't call setForcedSandboxFlags above. Yeah, this is only necessary if we try to remove force flags.
(In reply to comment #13) > > > Source/WebCore/dom/Document.cpp:1669 > > > + m_frame->loader()->setForcedSandboxFlags(SandboxOrigin); > > > > Is this part necessary? If you're going to set the security origin forcibly on the next line, you probably don't need this line. > > The next line only sets the flags on the Document, not on the FrameLoader. Why do we want to set it on the FrameLoader? It seems like setting it on the Document would be sufficient. Also, that should remove the need to clear the bit elsewhere.
Created attachment 94715 [details] Addressing abarth's comments
Comment on attachment 94715 [details] Addressing abarth's comments View in context: https://bugs.webkit.org/attachment.cgi?id=94715&action=review This is great. Thanks. > Source/WebCore/dom/Document.cpp:1725 > + if (!m_isViewSource || !m_frame) > + return; why !m_frame here?
(In reply to comment #16) > (From update of attachment 94715 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94715&action=review > > This is great. Thanks. > > > Source/WebCore/dom/Document.cpp:1725 > > + if (!m_isViewSource || !m_frame) > > + return; > > why !m_frame here? Left over from when I was using m_frame->loader() in the next line :) Will remove it before landing.
http://trac.webkit.org/changeset/87309