Bug 196280 – Web Inspector: Crash when interacting with Template Content in Console

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Crash when interacting with Template Content in Console

Steps to Reproduce:
1. Inspect data:text/html,<template>
2. Select "Template Content" node inside of the <template> element
3. js> $0.|
  => CRASH

Seems like the targetDocument->domWindow() is nullptr in WebCore::canAccessDocument?

Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000078)
[  0] 0x00007fff5253a12f WebCore`WebCore::DOMWindow::crossDomainAccessErrorMessage(WebCore::DOMWindow const&, WebCore::IncludeTargetOrigin) [inlined] WebCore::ContextDestructionObserver::scriptExecutionContext() const at ContextDestructionObserver.h:38:69
       34  	public:
       35  	    WEBCORE_EXPORT explicit ContextDestructionObserver(ScriptExecutionContext*);
       36  	    WEBCORE_EXPORT virtual void contextDestroyed();
       37  	
    -> 38  	    ScriptExecutionContext* scriptExecutionContext() const { return m_scriptExecutionContext; }
       39  	
       40  	protected:
       41  	    WEBCORE_EXPORT virtual ~ContextDestructionObserver();
       42  	    void observeContext(ScriptExecutionContext*);

[  0] 0x00007fff5253a12f WebCore`WebCore::DOMWindow::crossDomainAccessErrorMessage(WebCore::DOMWindow const&, WebCore::IncludeTargetOrigin) [inlined] WebCore::DOMWindow::document() const at DOMWindow.cpp:1435
       1431	}
       1432	
       1433	Document* DOMWindow::document() const
       1434	{
    -> 1435	    return downcast<Document>(ContextDestructionObserver::scriptExecutionContext());
       1436	}
       1437	
       1438	StyleMedia& DOMWindow::styleMedia()
       1439	{
    
[  0] 0x00007fff5253a12f WebCore`WebCore::DOMWindow::crossDomainAccessErrorMessage(WebCore::DOMWindow const&, WebCore::IncludeTargetOrigin) + 111 at DOMWindow.cpp:2210
       2206	    ASSERT(!activeWindow.document()->securityOrigin().canAccess(document()->securityOrigin()));
       2207	
       2208	    // FIXME: This message, and other console messages, have extra newlines. Should remove them.
       2209	    SecurityOrigin& activeOrigin = activeWindow.document()->securityOrigin();
    -> 2210	    SecurityOrigin& targetOrigin = document()->securityOrigin();
       2211	    String message;
       2212	    if (includeTargetOrigin == IncludeTargetOrigin::Yes)
       2213	        message = makeString("Blocked a frame with origin \"", activeOrigin.toString(), "\" from accessing a frame with origin \"", targetOrigin.toString(), "\". ");
       2214	    else
    
[  1] 0x00007fff5131b9e2 WebCore`WebCore::canAccessDocument(JSC::ExecState*, WebCore::Document*, WebCore::SecurityReportingOption) + 210 at JSDOMBindingSecurity.cpp:63:89
       59  	    case ThrowSecurityError:
       60  	        throwSecurityError(*state, scope, targetDocument->domWindow()->crossDomainAccessErrorMessage(active, IncludeTargetOrigin::No));
       61  	        break;
       62  	    case LogSecurityError:
    -> 63  	        printErrorMessageForFrame(targetDocument->frame(), targetDocument->domWindow()->crossDomainAccessErrorMessage(active, IncludeTargetOrigin::Yes));
       64  	        break;
       65  	    case DoNotReportSecurityError:
       66  	        break;
       67  	    }

[Yury Semikhatsky]

This happens because targetDocument->domWindow() is NULL:

#2  0x00007f09eaf54d9c in WebCore::DOMWindow::crossDomainAccessErrorMessage (this=0x0, activeWindow=...,                                │
    includeTargetOrigin=WebCore::IncludeTargetOrigin::Yes) at ../../Source/WebCore/page/DOMWindow.cpp:2271                              │

this is apparently because the template elements are created in the template document [1] which doesn't have a DOM window. I wonder why they have different security origins and how come scripts in the page don't hit the same check when they access the same template content. Any clue?


[1] https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/dom/Document.h#L1309

[Yury Semikhatsky]

Accessing the template element's content from a script in the same document doesn't require crossing frame boundaries, so it doesn't access any attributes marked as [CheckSecurityForNode] in .idl and hence BindingSecurity::checkSecurityForNode is not called in that scenario.

I see 2 possible fixes:
1) Inside InspectorDOMAgent::nodeAsScriptValue check if the node is from a template document and if so simply pass host document to the binding security check, instead of the node itself.

2) Update BindingSecurity::shouldAllowAccessToNode to use target->document()->templateDocumentHost() (if one is available) instead of target->document().

The former one looks safer as it will only affect inspector's code. Joe, what do you think?

[Joseph Pecoraro]

> this is apparently because the template elements are created in the template
> document [1] which doesn't have a DOM window. I wonder why they have
> different security origins and how come scripts in the page don't hit the
> same check when they access the same template content. Any clue?

Interesting. I was not aware of the special Template Document.

(In reply to Yury Semikhatsky from comment #2)
> Accessing the template element's content from a script in the same document
> doesn't require crossing frame boundaries, so it doesn't access any
> attributes marked as [CheckSecurityForNode] in .idl and hence
> BindingSecurity::checkSecurityForNode is not called in that scenario.

Makes sense.

> I see 2 possible fixes:
> 1) Inside InspectorDOMAgent::nodeAsScriptValue check if the node is from a
> template document and if so simply pass host document to the binding
> security check, instead of the node itself.
> 
> 2) Update BindingSecurity::shouldAllowAccessToNode to use
> target->document()->templateDocumentHost() (if one is available) instead of
> target->document().
> 
> The former one looks safer as it will only affect inspector's code. Joe,
> what do you think?

I'd go with the former as well for now. This particular check is originating from Inspector code anyways.

But it might be worth asking those familiar with the Template Document concept (likely Ryosuke). Can JavaScript ever get access to those nodes outside of Web Inspector?

[Joseph Pecoraro]

> > 2) Update BindingSecurity::shouldAllowAccessToNode to use
> > target->document()->templateDocumentHost() (if one is available) instead of
> > target->document().
> > 
> > The former one looks safer as it will only affect inspector's code. Joe,
> > what do you think?
> 
> I'd go with the former as well for now. This particular check is originating
> from Inspector code anyways.
> 
> But it might be worth asking those familiar with the Template Document
> concept (likely Ryosuke). Can JavaScript ever get access to those nodes
> outside of Web Inspector?

I just asked Ryosuke, and he had some good points.

  (1) The DOMWindow is only used for the `crossDomainAccessErrorMessage` string generation. Maybe accessing a DOMWindow could be eliminated entirely.
  (2) Having `canAccessDocument` itself handle template nodes would make sense.

If you avoid this crash, what happens with Web Inspector? Is a node actually get printed to the console and usable, or do you just get null back with a Security Error / exception?

[Yury Semikhatsky]

Created attachment 375225 [details]
sample page

[Yury Semikhatsky]

(In reply to Joseph Pecoraro from comment #4)
> If you avoid this crash, what happens with Web Inspector? Is a node actually
> get printed to the console and usable, or do you just get null back with a
> Security Error / exception?
The node is printed out and expandable. While writing a test I found another problem though, evaluating code like 'inspect(document.getElementsByTagName('template')[0].content.firstChild)' in the attached page will trigger errors in the frontend. I'll try to fix that too.

[Yury Semikhatsky]

Created attachment 375243 [details]
Patch

[Devin Rousso]

Comment on attachment 375243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375243&action=review

> Source/WebCore/bindings/js/JSDOMBindingSecurity.cpp:53
> +    if (targetDocument->templateDocumentHost())

Rather than call this twice, you could declare a variable inside the `if`:

```cpp
    if (auto* templateDocumentHost = targetDocument->templateDocumentHost())
        targetDocument = templateDocumentHost;
```

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2598
> +    if (document->templateDocumentHost())

Ditto (>JSDOMBindingSecurity.cpp:53)

> LayoutTests/inspector/dom/inspect-template-node.html:10
> +    InspectorProtocol.sendCommand("Runtime.evaluate", {"expression": "document.getElementsByTagName('template')[0].content.firstChild", "includeCommandLineAPI": true}, function(response) {

Can you give the `<template>` a unique id/class so it's easier to query for?

> LayoutTests/inspector/dom/inspect-template-node.html:12
> +        resolveNode(response.result.result.objectId);

Style: I know it would still work otherwise, but I prefer declaring variables/functions before they're used, so I'd reverse the order of all the functions.

> LayoutTests/inspector/dom/inspect-template-node.html:33
> +            if (response.result.result.value == "DIV") {
> +                ProtocolTest.log("PASS: $0 value is correct");

You can use `ProtocolTest.expectEqual(response.result.result.value, "DIV", "PASS: $0 value is correct")`.

> LayoutTests/inspector/dom/inspect-template-node.html:41
> +    function assertResponse(message, response) {

You can use `ProtocolTest.checkForError` instead of this.

> LayoutTests/inspector/dom/inspect-template-node.html:54
> +<body onload="runTest()">
> +</body>

Please include a `<p>...</p>` description of what this test is doing.

[Joseph Pecoraro]

Comment on attachment 375243 [details]
Patch

r=me with Devin's comments addressed

[Yury Semikhatsky]

Created attachment 375273 [details]
Patch

[Yury Semikhatsky]

(In reply to Devin Rousso from comment #8)
> Comment on attachment 375243 [details]

Addressed all comments. PTAL.

[Joseph Pecoraro]

Comment on attachment 375273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375273&action=review

> LayoutTests/inspector/dom/inspect-template-node-expected.txt:1
> +Test that document inside a template node can be passed to inspect() function in the console and refernced as $0.

Typo: "refernced" => "referenced"

> LayoutTests/inspector/dom/inspect-template-node-expected.txt:7
> +PASS: evaluate an element in a template
> +PASS: resolved js object id to DOM node id
> +PASS: set $0 to the template element
> +PASS: evaluate $0
> +PASS: $0 value is correct

Normally these lines would start with capital letters and end in a period. We aren't entirely consistent... but a capital letter would make the output read a bit better!

> LayoutTests/inspector/dom/inspect-template-node.html:9
> +    function assertResponse(message, response) {

I'd probably change the order of arguments here. (response, message), so the string is at the end like most other functions.

> LayoutTests/inspector/dom/inspect-template-node.html:10
> +        InspectorProtocol.checkForError(response)

Style: semicolon

> LayoutTests/inspector/dom/inspect-template-node.html:23
> +        InspectorProtocol.sendCommand("DOM.setInspectedNode", {nodeId}, function (response) {

Style: All of these `function (response) {` should not have a space. But you could just arrow function them `(response) => {`

[Yury Semikhatsky]

Created attachment 375314 [details]
test comments addressed

[Yury Semikhatsky]

(In reply to Joseph Pecoraro from comment #12)
> Comment on attachment 375273 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375273&action=review
> 
> > LayoutTests/inspector/dom/inspect-template-node-expected.txt:1
> > +Test that document inside a template node can be passed to inspect() function in the console and refernced as $0.
> 
> Typo: "refernced" => "referenced"
> 
> > LayoutTests/inspector/dom/inspect-template-node-expected.txt:7
> > +PASS: evaluate an element in a template
> > +PASS: resolved js object id to DOM node id
> > +PASS: set $0 to the template element
> > +PASS: evaluate $0
> > +PASS: $0 value is correct
> 
> Normally these lines would start with capital letters and end in a period.
> We aren't entirely consistent... but a capital letter would make the output
> read a bit better!
> 
Done.

> > LayoutTests/inspector/dom/inspect-template-node.html:9
> > +    function assertResponse(message, response) {
> 
> I'd probably change the order of arguments here. (response, message), so the
> string is at the end like most other functions.
>
Done.
 
> > LayoutTests/inspector/dom/inspect-template-node.html:10
> > +        InspectorProtocol.checkForError(response)
> 
> Style: semicolon
> 
Done.

> > LayoutTests/inspector/dom/inspect-template-node.html:23
> > +        InspectorProtocol.sendCommand("DOM.setInspectedNode", {nodeId}, function (response) {
> 
> Style: All of these `function (response) {` should not have a space. But you
> could just arrow function them `(response) => {`
Replaced with arrow functions for better readability. I wish something like check-webkit-style could catch formatting issues like these.

[Alex Christensen]

Comment on attachment 375314 [details]
test comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=375314&action=review

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2597
> +    Document* document = &node->document();

Fun fact: you can use std::reference_wrapper for all of your non-null reassignable pointer-like object needs. I'm not sure if it would make this code nicer, but it's worth considering.

[Yury Semikhatsky]

(In reply to Alex Christensen from comment #15)
> Comment on attachment 375314 [details]
> test comments addressed
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375314&action=review
> 
> > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2597
> > +    Document* document = &node->document();
> 
> Fun fact: you can use std::reference_wrapper for all of your non-null
> reassignable pointer-like object needs. I'm not sure if it would make this
> code nicer, but it's worth considering.

I feel like it'd be an overkill here given the limited scope of the local variable and would only hinder readability of the code. But if other reviewers feel strongly about it I can update the code.

[Joseph Pecoraro]

Comment on attachment 375314 [details]
test comments addressed

r=me! Thanks

[WebKit Commit Bot]

Comment on attachment 375314 [details]
test comments addressed

Clearing flags on attachment: 375314

Committed r248175: <https://trac.webkit.org/changeset/248175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53876640>