Bug 147962 – Web Inspector: $0 stops working after navigating to a different domain

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

[Nikita Vasilyev]

Steps:
1. Open http://webkit.org
2. Open Web Inspector
3. Go to http://apple.com
4. Enter $0 in the console

Expected:
DOM element is logged.

Actual:
Cross-origin error is thrown.
    Blocked a frame with origin "http://webkit.org" from accessing a frame with origin "https://apple.com".  The frame requesting access has a protocol of "http", the frame being accessed has a protocol of "https". Protocols must match.

[Nikita Vasilyev]

Created attachment 258861 [details]
Animated GIF of the problem

[Nikita Vasilyev]

Joe, you probably know how to fix this one.

[Radar WebKit Bug Importer]

<rdar://problem/22428685>

[Joseph Pecoraro]

<rdar://problem/21769029>

[Joseph Pecoraro]

Created attachment 264964 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 264971 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 264971 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6350
> +				A5AB49DA1BEC8079007020FB /* PerGlobalObjectWrapperWorld.cpp */,
> +				A5AB49DB1BEC8079007020FB /* PerGlobalObjectWrapperWorld.h */,
>  				A513E5CC185FB992007E95AD /* agents */,

Oops, I will fix sorting here.

[Joseph Pecoraro]

Created attachment 264973 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 264973 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:24:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 264978 [details]
[PATCH] Proposed Fix

Fixed Release build that needed JSCInlines include.

[Joseph Pecoraro]

Created attachment 265073 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 265073 [details]
[PATCH] Proposed Fix

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

r=me on tests. The wrapper parts make sense to me, but I would like Tim or someone on JSC team to take a glance in case I'm missing something, especially since this is essentially relaxing a security check.

> Source/JavaScriptCore/ChangeLog:10
> +        CommandLineAPIHost objects injected into multiple contexts.

I would add something about lifetimes. Relative to the previous wrapper scheme, this doesn't allow $0 or other host objects to live any longer, it just avoids cross-origin checks for subframes within one inspected page. Right?

> Source/JavaScriptCore/ChangeLog:13
> +        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:

Probably not necessary any more. These aren't used to build.

> Source/JavaScriptCore/ChangeLog:23
> +        Hold a bunch of per-global-object wrappers for an object

'for objects that may outlive'

> Source/JavaScriptCore/ChangeLog:26
> +        each execution context created by the page.

Maybe I'm jet lagged but it took me awhile to understand what this class does: it holds *one* arbitrary object wrapper (in this case, for CommandLineAPIHost) per global object. Maybe SingleWrapperPerGlobalObjectCache or some other name, or an introductory comment at the declaration, would be clearer.

> Source/JavaScriptCore/inspector/InjectedScriptHost.h:30
> +#include <inspector/PerGlobalObjectWrapperWorld.h>

Why angle brackets here?

> Source/WebCore/inspector/CommandLineAPIHost.cpp:156
> +    m_inspectedObject = std::make_unique<InspectableObject>();

It would be useful to explicitly comment that m_inspectedObject is typically bound to $0. It wasn't obvious until I read the changelog description of this method.

> Source/WebCore/inspector/CommandLineAPIHost.h:118
> +    Inspector::PerGlobalObjectWrapperWorld m_wrappers;

I had to look around this patch a lot before I realized that the only wrappers being tracked by this instance are multiple CommandLineAPIHost per each global objects. Maybe call this m_hostObjectWrappers or something.

[Joseph Pecoraro]

Comment on attachment 265073 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/inspector/InjectedScriptHost.h:30
>> +#include <inspector/PerGlobalObjectWrapperWorld.h>
> 
> Why angle brackets here?

Oops, not needed here in JSC. I had just copied/pasted it into here.

>> Source/WebCore/inspector/CommandLineAPIHost.cpp:156
>> +    m_inspectedObject = std::make_unique<InspectableObject>();
> 
> It would be useful to explicitly comment that m_inspectedObject is typically bound to $0. It wasn't obvious until I read the changelog description of this method.

The .h has "// $0" next to this instance variable.

[Joseph Pecoraro]

> r=me on tests. The wrapper parts make sense to me, but I would like Tim or
> someone on JSC team to take a glance in case I'm missing something,
> especially since this is essentially relaxing a security check.

This isn't relaxing a security check. The security check is checking if the the execution context where "$0" is evaluated has access to the node. However, previously, there was only ever one "CommandLineAPI" wrapper objects and it was treated as part of the domain/origin of the first execution context it was injected into. So when a sub-frame (or a new frame after navigation) tried to access $0 through this CommandLineAPI wrapper, it was errantly saying it was trying to access a node through cross origin means.

After this change, we make one wrapper per execution context. So when a specific execution context tries to access "$0" it uses a "CommandLineAPI" wrapper object associated with that execution context. The security check is still there, it just won't errantly happen as a result of the "CommandLineAPI" wrapper itself being created in a different execution context.


> > Source/JavaScriptCore/ChangeLog:10
> > +        CommandLineAPIHost objects injected into multiple contexts.
> 
> I would add something about lifetimes. Relative to the previous wrapper
> scheme, this doesn't allow $0 or other host objects to live any longer, it
> just avoids cross-origin checks for subframes within one inspected page.
> Right?

The C++ host object's lifetime doesn't change. It is alive as long as the Page is alive.

The only lifetime change is for the JSValue wrapper objects created for the Host object. Now, a wrapper get created for each execution context, essentially every time the CommandLineAPIModuleSource.js script is injected into a context, it gets a "CommandLineAPI" wrapper created for that context. Previously we created one wrapper and stored it in the global DOMWrapperWorld. Reusing it was preventing it from getting GC'd as well as the cross origin issue being fixed here. Now, the wrappers will get GC'd appropriately since we let them go during navigation / closing the Page.

[Blaze Burg]

(In reply to comment #14)
> > r=me on tests. The wrapper parts make sense to me, but I would like Tim or
> > someone on JSC team to take a glance in case I'm missing something,
> > especially since this is essentially relaxing a security check.
> 
> This isn't relaxing a security check. The security check is checking if the
> the execution context where "$0" is evaluated has access to the node.
> However, previously, there was only ever one "CommandLineAPI" wrapper
> objects and it was treated as part of the domain/origin of the first
> execution context it was injected into. So when a sub-frame (or a new frame
> after navigation) tried to access $0 through this CommandLineAPI wrapper, it
> was errantly saying it was trying to access a node through cross origin
> means.
> 
> After this change, we make one wrapper per execution context. So when a
> specific execution context tries to access "$0" it uses a "CommandLineAPI"
> wrapper object associated with that execution context. The security check is
> still there, it just won't errantly happen as a result of the
> "CommandLineAPI" wrapper itself being created in a different execution
> context.

I see, so it was checking origin of the wrapper object, which had an arbitrarily assigned first context. OK, this makes more sense now. Please incorporate some of this into the changelog.

> > > Source/JavaScriptCore/ChangeLog:10
> > > +        CommandLineAPIHost objects injected into multiple contexts.
> > 
> > I would add something about lifetimes. Relative to the previous wrapper
> > scheme, this doesn't allow $0 or other host objects to live any longer, it
> > just avoids cross-origin checks for subframes within one inspected page.
> > Right?
> 
> The C++ host object's lifetime doesn't change. It is alive as long as the
> Page is alive.
> 
> The only lifetime change is for the JSValue wrapper objects created for the
> Host object. Now, a wrapper get created for each execution context,
> essentially every time the CommandLineAPIModuleSource.js script is injected
> into a context, it gets a "CommandLineAPI" wrapper created for that context.
> Previously we created one wrapper and stored it in the global
> DOMWrapperWorld. Reusing it was preventing it from getting GC'd as well as
> the cross origin issue being fixed here. Now, the wrappers will get GC'd
> appropriately since we let them go during navigation / closing the Page.

Please also include some of this into the changelog. :)

r=me with the clarifications.

[Joseph Pecoraro]

> I see, so it was checking origin of the wrapper object, which had an
> arbitrarily assigned first context.

Yep!

[Joseph Pecoraro]

Created attachment 265105 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 265105 [details]
[PATCH] For Landing

Clearing flags on attachment: 265105

Committed r192186: <http://trac.webkit.org/changeset/192186>

[Nikita Vasilyev]

Not sure why this bug isn't closed, the patch has landed. Closing.