Bug 59113 – XMLViewer breaks when scripts are disabled

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

[Nate Chapin]

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

[Nate Chapin]

Created attachment 90553 [details]
Unique SecurityOrigins for view source documents

[Adam Barth]

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.

[Nate Chapin]

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 :)

[Adam Barth]

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?

[Nate Chapin]

(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 :)

[WebKit Commit Bot]

Comment on attachment 90760 [details]
Temporary fix

Clearing flags on attachment: 90760

Committed r84732: <http://trac.webkit.org/changeset/84732>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

Reopening, since the real fix is still in progress.

[Nate Chapin]

Created attachment 91361 [details]
Attempt #2 at unique SecurityOrigins

[Eric Seidel (no email)]

This is definitely a patch for abarth.

[Adam Barth]

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.

[Adam Barth]

Apparently I wrote that review a while ago and never clicked publish.  Thankfully Ojan's auto-saving kept it for me!

[Nate Chapin]

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

[Adam Barth]

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

[Nate Chapin]

Created attachment 94715 [details]
Addressing abarth's comments

[Adam Barth]

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?

[Nate Chapin]

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

[Nate Chapin]

http://trac.webkit.org/changeset/87309