Closed Bug 1240494 Opened 4 years ago Closed 4 years ago

Allow JSON Viewer to access shared UI components

Categories

(DevTools :: JSON Viewer, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: Honza, Assigned: jryans)

References

Details

Attachments

(2 files, 3 obsolete files)

JSON Viewer is currently using AMD so, every module is using:

define(function(require, exports, module) {
  // module 
});

Since we want to share code (mainly UI components in this case) JSON Viewer should adopt CommonJS syntax just like the rest of devtools code base.

One nice option is using webpack and generating a bundle file (with all modules included).

Honza
@Ryan: I know you don't like having another build-step (and I am not big fan of it either), but this is blocking us from sharing UI components defined by the viewer with the rest of the code base.
I think we should just go ahead and do it. Or do you see any other options?

Honza
Flags: needinfo?(jryans)
(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> @Ryan: I know you don't like having another build-step (and I am not big fan
> of it either), but this is blocking us from sharing UI components defined by
> the viewer with the rest of the code base.
> I think we should just go ahead and do it. Or do you see any other options?

Is there some technical reason that prevents your define addition to BrowserLoader from working?

In any case, I believe this should be discussed as a team.  Please start a thread about it on the mailing list.  It's a large change from how we work today, so the whole team should be involved in the discussion.

Since you have a timeline to meet, the define approach could be best for now, assuming that works.
Flags: needinfo?(jryans)
I thought Loader.jsm was working in content scope. I think it did at some point in the past.
The worker loader lives under similar constraints of content. The main issue about making our loader to work in content is its dependency to various Components.* stuff, which doesn't has the worker loader.
It may be worth looking if we can reuse worker loader from content:
  devtools/shares/worker/loader.js
It looks like it works if you expose some magic globals to it like "loadSubScript", which may be hard to implement in content.

It may be worth also looking into recent es6 modules progress, bug 1240072.
I would prefer us to assume our role of browser vendor and improve the web by dogfooding it rather than choosing one of its current workaround.
Given what has been done for the jsshell to expose a es6 module loader (bug 1215063), it doesn't seem very hard to expose a module loader, to chrome, at least.
As :dmose and :jlongster mentioned on the thread, SystemJS may be an option as a replacement for RequireJS.

It claims supports for all module formats:

https://github.com/systemjs/systemjs/blob/master/docs/module-formats.md

I would prefer we investigate this first before going down the build step road.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> As :dmose and :jlongster mentioned on the thread, SystemJS may be an option
> as a replacement for RequireJS.
> 
> It claims supports for all module formats:
> 
> https://github.com/systemjs/systemjs/blob/master/docs/module-formats.md
> 
> I would prefer we investigate this first before going down the build step
> road.
I looked at this and one issue I am seeing is that SystemJS is using XMLHttpRequest to fetch source of required modules. It doesn't work in our case since JSON Viewer modules come from resource://devtools/... -> causing access to restricted URI denied exception (same-origin policy violation).

Dan, is there any workaround for this?

Honza
Flags: needinfo?(dmose)
(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > As :dmose and :jlongster mentioned on the thread, SystemJS may be an option
> > as a replacement for RequireJS.
> > 
> > It claims supports for all module formats:
> > 
> > https://github.com/systemjs/systemjs/blob/master/docs/module-formats.md
> > 
> > I would prefer we investigate this first before going down the build step
> > road.
> I looked at this and one issue I am seeing is that SystemJS is using
> XMLHttpRequest to fetch source of required modules. It doesn't work in our
> case since JSON Viewer modules come from resource://devtools/... -> causing
> access to restricted URI denied exception (same-origin policy violation).

I first tried to see if web content could load an iframe from resource://devtools/, but that's blocked (as I sort of expected).  If it had worked, then we JSON Viewer could have been loaded from inside an iframe where further resources would be same-origin.

Another path would be to start an in-product HTTP server, possibly using one of the in-tree httpd.js servers, or something else.  This would let us set CORS headers on DevTools resources to allow cross-origin requests.
I haven't actually used SystemJS, but https://github.com/systemjs/systemjs/issues/750 contains some tantalizing hints, perhaps using the scriptLoad thing, or the CORS loader, if that still exists, could be helpful.

Alternately, they do have a plugin API:

https://github.com/systemjs/systemjs/blob/master/docs/creating-plugins.md

It might (using the "fetch" hook?) be straightforward to write a plugin to use something besides XHR.
Flags: needinfo?(dmose)
When I wrote "CORS loader", I actually meant "CSP loader".
Attached patch bug1240494.patch (obsolete) — Splinter Review
(In reply to Dan Mosedale (:dmose) from comment #8)
> I haven't actually used SystemJS, but
> https://github.com/systemjs/systemjs/issues/750 contains some tantalizing
> hints, perhaps using the scriptLoad thing, or the CORS loader, if that still
> exists, could be helpful.
> 
> Alternately, they do have a plugin API:
> 
> https://github.com/systemjs/systemjs/blob/master/docs/creating-plugins.md
> 
I couldn't make the 'scriptLoad' flag working. When this is set plain <script> injection is used to fetch the source and in such case there is no control over the fetched source that is evaluated immediately (no way to append wrapper around). But perhaps I am missing something.

> It might (using the "fetch" hook?) be straightforward to write a plugin to
> use something besides XHR.
I don't know what else we could use instead of XHR.


(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Another path would be to start an in-product HTTP server, possibly using one
> of the in-tree httpd.js servers, or something else.  This would let us set
> CORS headers on DevTools resources to allow cross-origin requests.
I tried this and it works (see attached patch), but I am not sure about security implications. This way we are basically opening up devtools source to the world...

Honza
It looks like we are trying to do within regular content privileges more than what we can do with commong web privileges.
It looks like, this isn't about resource:// being special but more about a simple CORS issue. Like requesting between two domains.
Even if we end up serving via a local http, we would have to also send the right http headers to allow cross domain requests.
I'm not a big fan of starting a http server in firefox just for that.

For me, it looks like we should give more privileges to json viewer document or change its domain so that it can request the modules from resource://devtools.

Unfortunately, I don't see how this feature work. I'm not able to get the json to be pretty printed, even with the very special mime type we are using.
But may be something like this will make it work:
diff --git a/devtools/client/jsonview/converter-child.js b/devtools/client/jsonview/converter-child.js
index 53a1993..7ba484c 100644
--- a/devtools/client/jsonview/converter-child.js
+++ b/devtools/client/jsonview/converter-child.js
@@ -103,6 +103,8 @@ var Converter = Class({
     this.channel = aRequest;
     this.channel.contentType = "text/html";
     this.channel.contentCharset = "UTF-8";
+    this.channel.owner = Services.scriptSecurityManager
+      .createCodebasePrincipal(Services.io.newURI("resource://devtools/", null, null), {});
 
     this.listener.onStartRequest(this.channel, aContext);
   },
(In reply to Jan Honza Odvarko [:Honza] from comment #10)
>
> > It might (using the "fetch" hook?) be straightforward to write a plugin to
> > use something besides XHR.

> I don't know what else we could use instead of XHR.

I imagine you could reimplement the scriptLoad flag (presumably by injecting <script> tags into the DOM) using a plugin, if it can't be made to work otherwise.
(In reply to Alexandre Poirot [:ochameau] from comment #11)
> It looks like we are trying to do within regular content privileges more
> than what we can do with commong web privileges.
> It looks like, this isn't about resource:// being special but more about a
> simple CORS issue. Like requesting between two domains.
> Even if we end up serving via a local http, we would have to also send the
> right http headers to allow cross domain requests.
> I'm not a big fan of starting a http server in firefox just for that.
> 
> For me, it looks like we should give more privileges to json viewer document
> or change its domain so that it can request the modules from
> resource://devtools.
> 
> Unfortunately, I don't see how this feature work. I'm not able to get the
> json to be pretty printed, even with the very special mime type we are using.
Make sure you have devtools.jsonview.enabled set to true.

Does it help?

> But may be something like this will make it work:
> diff --git a/devtools/client/jsonview/converter-child.js
> b/devtools/client/jsonview/converter-child.js
> index 53a1993..7ba484c 100644
> --- a/devtools/client/jsonview/converter-child.js
> +++ b/devtools/client/jsonview/converter-child.js
> @@ -103,6 +103,8 @@ var Converter = Class({
>      this.channel = aRequest;
>      this.channel.contentType = "text/html";
>      this.channel.contentCharset = "UTF-8";
> +    this.channel.owner = Services.scriptSecurityManager
> +      .createCodebasePrincipal(Services.io.newURI("resource://devtools/",
> null, null), {});
>  
>      this.listener.onStartRequest(this.channel, aContext);
>    },
Still doesn't work for me, but the problem seems to be a bit different now. Instead of getting "Access to restricted URI denied" I am just getting an empty XHR response.


In any case I found another problem with this solution (and also the in-product server solution)
Loading this page: https://api.github.com/orgs/firebug/repos fires:

Error: "Content Security Policy: The page's settings blocked the loading of a resource at self ("default-src 'none'")


Honza
Flags: needinfo?(poirot.alex)
Attached patch change principal and csp (obsolete) — Splinter Review
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> Make sure you have devtools.jsonview.enabled set to true.
> 
> Does it help?

Yes!

> Still doesn't work for me, but the problem seems to be a bit different now.
> Instead of getting "Access to restricted URI denied" I am just getting an
> empty XHR response.

I don't know if testing in the webconsole is like executing code within the html,
but it seems to work if you open a json and eval this in its web console:

x = new XMLHttpRequest(); x.open("GET", "resource://devtools/client/framework/gDevTools.jsm", false); x.send(); x.responseText;

> In any case I found another problem with this solution (and also the
> in-product server solution)
> Loading this page: https://api.github.com/orgs/firebug/repos fires:
> 
> Error: "Content Security Policy: The page's settings blocked the loading of
> a resource at self ("default-src 'none'")

But I can see these CSP exception. I see one when I try to eval("") from the web console...

This new patch should fix that. This is a quick'n dirty patch,
we would have to store headers in a weakmap of channels or something alike.
Flags: needinfo?(poirot.alex)
The issue with CSP is that the github page returns some CSP headers and ends up changing the permission of our code. The whole view-source thing, with the channel hacks seems weak. It is very disturbing to know that the targeted website can end up changing our browser code behavior.
We could potentially use an expanded principal with both resource://devtools and the page's principal together.  Add-on SDK uses this approach with content scripts.

SDK also mentions needing to replace[1] XHR with a special version that does the right security checks.

Would need to test what access the page would have though...  I assume if it only has resource://devtools and not chrome://, that should restrict what a malicious page could do.

[1]: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/addon-sdk/source/lib/sdk/content/sandbox.js#134-137
I'm not sure we even need page principal? Don't we pass the json content, headers from converter-child?
But in case we do, yes, expanded principal may help. I don't know exactly about XHR, the SDK comments are most likely related to the fact it's using Sandboxes which don't have XHR exposed by default. Here we already have an XHR, but I don't know if it will check against the expanded principal or somehow check about another one.

Honza, if you are still having issue, please do not hesitate to attach a sketch of what you are doing so that I can bullet proof everything and you can resume doing some real things.
Attached patch bug1240494.patch (obsolete) — Splinter Review
I am blocked by this issue on almost every bug I am working on and I need to solve it ASAP. I created a patch that uses webpack to build bundle.js from all included modules and so, AMD syntax isn't needed.

I am happy if someone can come up with a solution where webpack isn't needed, whatever it is or wait till ES modules are nativelly supported and remove webpack again. But for now I just need to move forward with other things.


Try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=573fb60adb4f

Honza
Attachment #8713154 - Attachment is obsolete: true
Attachment #8719149 - Flags: review?(jryans)
Comment on attachment 8719149 [details] [diff] [review]
bug1240494.patch

Review of attachment 8719149 [details] [diff] [review]:
-----------------------------------------------------------------

I still feel strongly that this should only be done if we have exhausted all other alternatives.  So far, I don't believe we've reached that point.

Here are a few reasons why I dislike this:

1. We are committing build artifacts into the tree
  * It seems easy for bundle.js to accidentally get out of sync from the original source files
  * Reviewers may forget to verify that all changes to source files also appear in the output bundle, even though the output bundle is only thing that actually runs
  * This could be solved by running the build step as part of the Firefox build process (though I would still personally prefer it not be there at all)

2. We have expended a lot of effort in the last few quarters so that our code is easy to follow, easy to reload, and runs directly from source files with no steps needed after "Save".  Any kind of build step adds complexity and moves us further away from this goal, not closer to it.

3. I worry that by allowing a build step in this case, we will be even more likely to use one in other places too.  This would drive us even further away from what we've achieved lately with ease of running and reloading.

Since you've requested my review here, I will mark r- for the reasons above.

However, I know not everyone on the team feels the same way, including :jwalker.  So, I'll request his review, since he is the DevTools module owner.

::: devtools/client/jsonview/converter-child.js
@@ +207,5 @@
>        '<head><title>' + this.htmlEncode(title) + '</title>' +
>        '<base href="' + this.htmlEncode(baseUrl) + '">' +
>        '<link rel="stylesheet" type="text/css" href="' + themeVarsUrl + '">' +
>        '<link rel="stylesheet" type="text/css" href="css/main.css">' +
> +      '<script data-main="viewer-config" src="resource://devtools/client/shared/vendor/react.js"></script>' +

`data-main` is specific to require.js, it seems unneeded for all of these script tags.
Attachment #8719149 - Flags: review?(jwalker)
Attachment #8719149 - Flags: review?(jryans)
Attachment #8719149 - Flags: review-
I'm on board with build steps being something to avoid, but I don't want to let the best be the enemy of the good.

I want to make sure that we have a better solution to this problem very soon - I've been muttering about doing devtools.html for real in Q2, and we'll need to solve this properly for then.

I agree that we've not exhausted all other alternatives, but I think we've done what we can without doing Real Loader Work [1].

So the broad picture for me is; solving the loader problem is top-priority going into Q2, and getting all the disparate parts of Firebug.next landed is top-priority now.

So the choices are:

1. Solve this properly [2] in a week and ask Honza to work on other things in the mean time.
2. Solve this as a follow-up bug that we can use for our work in Q2.

I'm arguing that 2 is a better solution for now.


[1]. Where Real Loader Work means engineering that takes more than a week - i.e. it's not just learning how to configure webpack/systemjs/requirejs/etc
[2]. Where 'properly' means that it's a solution that we're happy to use for all our stuff as we move it into content.
:Honza, :bgrins, :jwalker, and I discussed this over Vidyo today.  We weighed the pros and cons of different approaches, and made some notes about them:

https://public.etherpad-mozilla.org/p/EPAZBv9f4j

Long term, we definitely want to have a good loader / module story for content priv. to support devtools.html.  (Though, as :bgrins mentioned, stage 1 of devtools.html may just be XUL -> HTML, while keeping chrome priv. at first until a later stage.)  I have filed bug 1248830 to work on our long term plans.

The immediate problem faced in this bug is there are UI components inside JSON Viewer (content priv.) that we want to share with other tools (chrome priv.).  JSON Viewer cannot use either the traditional DevTools loader or the newer BrowserLoader, as both require chrome priv.  

We also agreed that it is valuable to avoid a build step for this problem, as long as there is still a short term way to make progress on the work currently blocked here.

So, in the short term, we'll use a UMD-like wrapper for JSON Viewer *and* the shared modules it needs.  Hopefully it won't need to spread too far beyond that set of modules.  Also, we don't need "full" UMD, as we only need to support AMD (for RequireJS in the JSON Viewer) and CommonJS (for chrome priv. loaders we use elsewhere).

I decided to make the needed changes myself, so I could be sure this is a workable approach.  I tried the following quick verification:

* JSON Viewer was able to load AMD format of devtools/client/shared/components/tree.js after adding wrapper
  * I did have to remove the ViewHelpers.jsm import, will need to avoid JSMs for shared modules
* Memory tool can still use the same component in CJS format
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
QA Contact: jryans
Summary: JSON Viewer: use webpack to avoid AMD → Allow JSON Viewer to access shared UI components
Assignee: odvarko → jryans
QA Contact: jryans
Attachment #8719149 - Attachment is obsolete: true
Attachment #8719149 - Flags: review?(jwalker)
Attachment #8714445 - Attachment is obsolete: true
Comment on attachment 8720082 [details]
MozReview Request: Bug 1240494 - AMD support for DevTools loaders. r=jwalker,Honza

https://reviewboard.mozilla.org/r/35199/#review31885
Attachment #8720082 - Flags: review?(jwalker) → review+
Attachment #8720083 - Flags: review?(odvarko) → review+
Comment on attachment 8720083 [details]
MozReview Request: Bug 1240494 - Expand JSON Viewer paths to all shared. r=Honza

https://reviewboard.mozilla.org/r/35201/#review31893
Attachment #8720082 - Flags: review?(odvarko) → review+
Comment on attachment 8720082 [details]
MozReview Request: Bug 1240494 - AMD support for DevTools loaders. r=jwalker,Honza

https://reviewboard.mozilla.org/r/35199/#review31895
https://hg.mozilla.org/mozilla-central/rev/66b04591ffd7
https://hg.mozilla.org/mozilla-central/rev/54def3592f85
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.