Bug 59113 - XMLViewer breaks when scripts are disabled
Summary: XMLViewer breaks when scripts are disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-21 10:26 PDT by Nate Chapin
Modified: 2011-05-25 11:51 PDT (History)
5 users (show)

See Also:


Attachments
Unique SecurityOrigins for view source documents (6.90 KB, patch)
2011-04-21 10:32 PDT, Nate Chapin
abarth: review-
Details | Formatted Diff | Diff
Temporary fix (1.58 KB, patch)
2011-04-22 14:25 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Attempt #2 at unique SecurityOrigins (9.64 KB, patch)
2011-04-27 15:29 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Addressing abarth's comments (8.36 KB, patch)
2011-05-24 16:57 PDT, Nate Chapin
abarth: review+
japhet: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2011-04-21 10:26:55 PDT
(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?
Comment 1 Nate Chapin 2011-04-21 10:32:58 PDT
Created attachment 90553 [details]
Unique SecurityOrigins for view source documents
Comment 2 Adam Barth 2011-04-21 11:41:12 PDT
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.
Comment 3 Nate Chapin 2011-04-22 14:25:25 PDT
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 4 Adam Barth 2011-04-22 14:41:12 PDT
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?
Comment 5 Nate Chapin 2011-04-22 14:48:41 PDT
(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 6 WebKit Commit Bot 2011-04-22 20:20:18 PDT
Comment on attachment 90760 [details]
Temporary fix

Clearing flags on attachment: 90760

Committed r84732: <http://trac.webkit.org/changeset/84732>
Comment 7 WebKit Commit Bot 2011-04-22 20:20:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Nate Chapin 2011-04-27 10:47:42 PDT
Reopening, since the real fix is still in progress.
Comment 9 Nate Chapin 2011-04-27 15:29:42 PDT
Created attachment 91361 [details]
Attempt #2 at unique SecurityOrigins
Comment 10 Eric Seidel (no email) 2011-05-23 15:52:02 PDT
This is definitely a patch for abarth.
Comment 11 Adam Barth 2011-05-23 15:56:55 PDT
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.
Comment 12 Adam Barth 2011-05-23 15:57:35 PDT
Apparently I wrote that review a while ago and never clicked publish.  Thankfully Ojan's auto-saving kept it for me!
Comment 13 Nate Chapin 2011-05-24 15:45:10 PDT
> > 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.
Comment 14 Adam Barth 2011-05-24 16:37:51 PDT
(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.
Comment 15 Nate Chapin 2011-05-24 16:57:37 PDT
Created attachment 94715 [details]
Addressing abarth's comments
Comment 16 Adam Barth 2011-05-24 17:06:07 PDT
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?
Comment 17 Nate Chapin 2011-05-25 10:53:45 PDT
(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.
Comment 18 Nate Chapin 2011-05-25 11:51:45 PDT
http://trac.webkit.org/changeset/87309