Web Inspector: [REGRESSION] Double Clicking Resources Fails to Open in New Window

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Steps to Reproduce:
1. Open the Inspector
2. Select the Resources Panel
3. Double click a Resource in the sidebar (a .js or .css is good)

Expected Results:
A new window should open with that resource.

Actual Results:
A blank window.

Basics:
The fix for this is actually rather simple.  "this.resource.url" was change to "this.resource._url", some time ago and its used 3 times still in ResourcesPanel.js.  However, InjectedScriptAccess calls _require_ a callback function which needed to be added.

InjectedScriptAccess:
The above fix produces a new problem.  Inside InjectedScriptAccess._installHandler the "result" after the window.open() call is the String => "undefined".  This causes problems with JSON.parse.

> JSON.parse("undefined")
Exception: SyntaxError: Unable to parse JSON string
> JSON.parse('"undefined"')
"undefined" // string type

Where would this best be handled? Before pushing to the front end or right inside the _installHandler before JSON.parse()?

[Pavel Feldman]

Created attachment 40170 [details]
patch

[Pavel Feldman]

Created attachment 40171 [details]
same with error report

[Pavel Feldman]

> The fix for this is actually rather simple.  "this.resource.url" was change to
> "this.resource._url", some time ago and its used 3 times still in
> ResourcesPanel.js.  

url is a getter for _url on Resource.

> However, InjectedScriptAccess calls _require_ a callback
> function which needed to be added.
> 
> InjectedScriptAccess:
> The above fix produces a new problem.  Inside
> InjectedScriptAccess._installHandler the "result" after the window.open() call
> is the String => "undefined".  This causes problems with JSON.parse.
> 
> > JSON.parse("undefined")
> Exception: SyntaxError: Unable to parse JSON string
> > JSON.parse('"undefined"')
> "undefined" // string type
> 
> Where would this best be handled? Before pushing to the front end or right
> inside the _installHandler before JSON.parse()?

I think we should fix it on the stringify side (see latest patch).

[Joseph Pecoraro]

(In reply to comment #3)

> url is a getter for _url on Resource.

Ahh. I was getting undefined because I hadn't yet provided a callback parameter!  I appreciate the extra comments in InjectedScriptAccess.

> > Where would this best be handled? Before pushing to the front end or right
> > inside the _installHandler before JSON.parse()?
> 
> I think we should fix it on the stringify side (see latest patch).

Perfect. The patch works great =)


Was your window.open() really getting blocked so you had to move to the eval? Personally I think the eval is ugly and I can't believe using eval is enough to trick the blockers!  Both work on my end.

[Pavel Feldman]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > url is a getter for _url on Resource.
> 
> Ahh. I was getting undefined because I hadn't yet provided a callback
> parameter!  I appreciate the extra comments in InjectedScriptAccess.
> 
> > > Where would this best be handled? Before pushing to the front end or right
> > > inside the _installHandler before JSON.parse()?
> > 
> > I think we should fix it on the stringify side (see latest patch).
> 
> Perfect. The patch works great =)
> 
> 
> Was your window.open() really getting blocked so you had to move to the eval?
> Personally I think the eval is ugly and I can't believe using eval is enough to
> trick the blockers!  Both work on my end.

Hm... Eval is ugly, but open() did not work for me. I just tracked it to JSDOMWindowCustom.cpp:802 (allowPopUp check) and switched to eval.

The reason I gave up so easily was that the window object we are interacting with is a quarantine wrapper around the real window. As a result, _window().open and _window().eval("window.open") are invoking open on different JS objects. Now that you say that open() works for you, I'll dig a bit more...

[Pavel Feldman]

In my case, eval was having active event and action was recognized to be user gesture, whereas open was loosing the active event. That does not help though since we'd like to establish async interaction between the frontend and the injected script.

I can't use inspector's window.open since its chrome won't create me a new window for me. As a result, I can only think of native implementation for openInInspectedWindow in InspectorController (or even delegating into InspectorClient in case I need it to be platform-specific). Timothy, do you have any hints for me?

[Joseph Pecoraro]

> Now that you say that open() works for you, I'll dig a bit more...

I looked on my end and realized when I tested the patch I didn't enable "block popups".  You're right, when blocking popups is enabled the _window.open() doesn't work.  In that case I think the eval is fine, especially since you provided a comment explaining why.  Ultimately it would be nice to have a non-eval solution though.

Thanks for doing this on a Saturday!

[Timothy Hatcher]

I am surprised eval works. This should become clear once we drop quarantine wrappers.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/InjectedScriptAccess.js
	M	WebCore/inspector/front-end/ResourcesPanel.js
Committed r48797