Closed Bug 1466534 Opened 6 years ago Closed 6 years ago

[Style editor] Not showing content in new tab from local files

Categories

(DevTools :: Style Editor, defect, P1)

61 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61+ wontfix, firefox62+ verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 + wontfix
firefox62 + verified

People

(Reporter: cfogel, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image 1.png
[Affected versions]:
- Firefox 61.0b10, Firefox 62.0a1

[Affected platforms]:
- Windows 10x64, Ubuntu 16.04LTS, MacOS10.13.4

Notes:
Prerequisites: a basic html template with included .css file saved locally 
ex: https://drive.google.com/file/d/1BWjz2c6tB0fKfMG51B26CYfJ5haFH5E8/view

[Steps to reproduce]:
1. Launch Firefox;
2. Open the .html file in the browser;
3. Press Shift+F7 on the keyboard;
4. Right click on any .css file listed;
5. Click on the [Open in a new tab] option.

[Expected result]:
- The content of the .css file is displayed in the new tab.

[Actual result]:
- The new tab is empty.

[Regression range]:
- Mozregression points out to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1451976
- pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=73a3eabd2bd29d2ef8945648615f49ed29b326c9&tochange=b9f53df7f40072fc40126914778d82ab4a6c4b3a

[Additional notes]:
- Issue reproducible for files saved locally, the tab seems to hang; inspector can only be opened from the keyboard shortcuts;
- The generated link is working if opened in a new tab;  
- Attached screenshot with the issue;
Blocks: 1451976
Flags: needinfo?(mgaudet)
I don't think this is me: It reproduces for me with https://hg.mozilla.org/integration/mozilla-inbound/rev/b777948153fc by launching via  mozregression --launch b777948153fc; this is relevant because that is the revision that backed out the push in the range pointed to by mozregression.

Potentially another change in that range?
Flags: needinfo?(mgaudet)
No longer blocks: 1451976
Flags: needinfo?(cristian.fogel)
Double checked with a colleague on another workstation and a MacOS.

We both came up with the following:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=70ab5d0f6da37783913e39857edf488ccfc2ab7b&tochange=de33fb39fa4878714120d06ce6a0c72179016be9

This should help out a bit more.
Flags: needinfo?(cristian.fogel)
Yep, that makes a lot more sense.
Blocks: 1374741
Flags: needinfo?(jkt)
Ryan, do you have cycles to look at this by chance? We're down to our last 2 betas before the RC build on the 18th :(
Flags: needinfo?(jryans)
It's not clear to me how this case is supposed to work under the new API, so I think we need more input from :jkt here.  We're using `openWebLinkIn` here[1] because the sheet URLs come from content, which seems likely the intended usage of the API.

Does it means all file:// URIs all blocked via this API?  If so, we may need a different approach for links in DevTools.

[1]: https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/devtools/client/styleeditor/StyleEditorUI.jsm#490
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> It's not clear to me how this case is supposed to work under the new API, so
> I think we need more input from :jkt here.  We're using `openWebLinkIn`
> here[1] because the sheet URLs come from content, which seems likely the
> intended usage of the API.
> 
> Does it means all file:// URIs all blocked via this API?  If so, we may need
> a different approach for links in DevTools.
> 
> [1]:
> https://searchfox.org/mozilla-central/rev/
> edbf2c009992315d85eeb885e1b8edbbd43c84b7/devtools/client/styleeditor/
> StyleEditorUI.jsm#490

You can fix this by passing the principal of the page that's included the style sheets, like:

openWebLinkIn(url, "tab", {triggeringPrincipal: document.nodePrincipal})

assuming you have access to the document and/or pass its principal elsewhere. Does that help?
Flags: needinfo?(jkt) → needinfo?(jryans)
(In reply to :Gijs (he/him) from comment #6)
> You can fix this by passing the principal of the page that's included the
> style sheets, like:
> 
> openWebLinkIn(url, "tab", {triggeringPrincipal: document.nodePrincipal})
> 
> assuming you have access to the document and/or pass its principal
> elsewhere. Does that help?

Ah, I see.  Unfortunately, DevTools doesn't currently know the document's principal directly, so it's a bit trickier...

DevTools generally tries to assume the tool UI and content being targeted may be remote from each other to support remote debugging.  The client (who wants to open a link) could ask the server (where the content is) for the document's principal.  Perhaps with some validation that the server isn't malicious and saying "system principal" for everything, it could be okay?  But then if you're debugging with Browser Toolbox, the documents actually are loaded with system principal, so that might need a special case...  Is there any prior art that would help guide us here?

As a short term fix, we can special case local debugging and use the tab's contentPrincipal as the triggeringPrincipal.  It's only correct for the top-level document though, I suppose.  Does that seem okay for now?
Flags: needinfo?(jryans) → needinfo?(gijskruitbosch+bugs)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> (In reply to :Gijs (he/him) from comment #6)
> > You can fix this by passing the principal of the page that's included the
> > style sheets, like:
> > 
> > openWebLinkIn(url, "tab", {triggeringPrincipal: document.nodePrincipal})
> > 
> > assuming you have access to the document and/or pass its principal
> > elsewhere. Does that help?
> 
> Ah, I see.  Unfortunately, DevTools doesn't currently know the document's
> principal directly, so it's a bit trickier...

Ah, right.

> DevTools generally tries to assume the tool UI and content being targeted
> may be remote from each other to support remote debugging.  The client (who
> wants to open a link) could ask the server (where the content is) for the
> document's principal.  Perhaps with some validation that the server isn't
> malicious and saying "system principal" for everything, it could be okay? 
> But then if you're debugging with Browser Toolbox, the documents actually
> are loaded with system principal, so that might need a special case...  Is
> there any prior art that would help guide us here?

I'm not sure. In a sense this seems like a devtools-specific problem... so not sure what prior art you'd be looking for?

In theory the principals should also help with things like containers. I don't know if they currently do, but the information is there for them to do that. System principal doesn't have any of that information, so in that sense it's strictly worse (that and it allows opening 'whatever' which is always a bad idea...).

> As a short term fix, we can special case local debugging and use the tab's
> contentPrincipal as the triggeringPrincipal.  It's only correct for the
> top-level document though, I suppose.  Does that seem okay for now?

That seems certainly no worse than assuming system principal, so sure.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Priority: -- → P1
I filed bug 1467945 to investigate a proper fix for this.

For now, I'll use the tab's contentPrincipal where needed.
Blocks: 1467945
Product: Firefox → DevTools
Comment on attachment 8984936 [details]
Bug 1466534 - Centralize DevTools link handling.

https://reviewboard.mozilla.org/r/250720/#review257218

Thanks for fixing this! Some nits, and I would really like us to simplify shared/link.js but this can be moved to a followup.

::: devtools/client/dom/dom-panel.js
(Diff revision 1)
>      return deferred.promise;
>    },
>  
>    openLink: function(url) {
> -    const parentDoc = this._toolbox.doc;
> +    openContentLink(url);
> -    const iframe = parentDoc.getElementById("this._toolbox");

interesting to see that this was broken before!

::: devtools/client/inspector/computed/computed.js:1196
(Diff revision 1)
>     */
>    mdnLinkClick: function(event) {
>      const inspector = this.tree.inspector;
>  
>      if (inspector.target.tab) {
> -      const browserWin = inspector.target.tab.ownerDocument.defaultView;
> +      openContentLink(this.link);

I think we could remove the if() statement here, we no longer need inspector.target.tab.

::: devtools/client/inspector/inspector.js:38
(Diff revision 1)
>  loader.lazyRequireGetter(this, "Menu", "devtools/client/framework/menu");
>  loader.lazyRequireGetter(this, "MenuItem", "devtools/client/framework/menu-item");
>  loader.lazyRequireGetter(this, "ExtensionSidebar", "devtools/client/inspector/extensions/extension-sidebar");
>  loader.lazyRequireGetter(this, "CommandUtils", "devtools/client/shared/developer-toolbar", true);
>  loader.lazyRequireGetter(this, "clipboardHelper", "devtools/shared/platform/clipboard");
> +loader.lazyRequireGetter(this, "openContentLink", "devtools/client/shared/link", true);

I think :gl tries to keep the imports sorted alphabetically in this file, so we should move this one line up.

::: devtools/client/menus.js:136
(Diff revision 1)
>      id: "devToolsEndSeparator"
>    },
>    { id: "getMoreDevtools",
>      l10nKey: "getMoreDevtoolsCmd",
>      oncommand(event) {
> -      const window = event.target.ownerDocument.defaultView;
> +      openDocLink("https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/", "tab");

We should remove this "tab" parameter

::: devtools/client/shared/link.js:27
(Diff revision 1)
>   * @param {String} url
>   *        The url to open.
>   * @param {Object} options
>   *        Optional parameters, see documentation for openUILinkIn in utilityOverlay.js
>   */
> -exports.openWebLink = async function(url, options) {
> +exports.openDocLink = async function(url, options) {

OK to land as is since it is a P1, but as discussed during the all hands I am not sure about the current split between openDocLink and openContentLink.

I already feel that openWeb/openTrusted is confusing enough (cf our previous questions about what should be used when) and we should not add more complexity here.

I would propose to stick with the names available on window. ie openTrustedLink, openWebLink. And either add a devtools specific option "useContentPrincipal" or have a third method openWebLinkWithContentPrincipal. In both cases the comment should explain in which situations using the content principal is needed. Or just make it the default behavior for DevTools' openWebLink(). I don't think there is any harm in using the contentPrincipal when opening a hardcoded documentation link?
Attachment #8984936 - Flags: review?(jdescottes) → review+
Comment on attachment 8984936 [details]
Bug 1466534 - Centralize DevTools link handling.

https://reviewboard.mozilla.org/r/250720/#review257218

> I think we could remove the if() statement here, we no longer need inspector.target.tab.

Thanks, good catch!

> We should remove this "tab" parameter

Thanks!

> OK to land as is since it is a P1, but as discussed during the all hands I am not sure about the current split between openDocLink and openContentLink.
> 
> I already feel that openWeb/openTrusted is confusing enough (cf our previous questions about what should be used when) and we should not add more complexity here.
> 
> I would propose to stick with the names available on window. ie openTrustedLink, openWebLink. And either add a devtools specific option "useContentPrincipal" or have a third method openWebLinkWithContentPrincipal. In both cases the comment should explain in which situations using the content principal is needed. Or just make it the default behavior for DevTools' openWebLink(). I don't think there is any harm in using the contentPrincipal when opening a hardcoded documentation link?

Hmm... Well, I do agree it's confusing, and maybe this doesn't make it less confusing... With this patch, I am mainly trying to ensure DevTools uses the least privileged principal needed to open the respective page.

For the moment, I am not quite sure if better method names, documentation, or something else would be best. Anyway, I filed bug 1469655 to follow up.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> Comment on attachment 8984936 [details]
> Bug 1466534 - Centralize DevTools link handling.
> 
> https://reviewboard.mozilla.org/r/250720/#review257218
> 
> > I think we could remove the if() statement here, we no longer need inspector.target.tab.
> 
> Thanks, good catch!
> 
> > We should remove this "tab" parameter
> 
> Thanks!
> 
> > OK to land as is since it is a P1, but as discussed during the all hands I am not sure about the current split between openDocLink and openContentLink.
> > 
> > I already feel that openWeb/openTrusted is confusing enough (cf our previous questions about what should be used when) and we should not add more complexity here.
> > 
> > I would propose to stick with the names available on window. ie openTrustedLink, openWebLink. And either add a devtools specific option "useContentPrincipal" or have a third method openWebLinkWithContentPrincipal. In both cases the comment should explain in which situations using the content principal is needed. Or just make it the default behavior for DevTools' openWebLink(). I don't think there is any harm in using the contentPrincipal when opening a hardcoded documentation link?
> 
> Hmm... Well, I do agree it's confusing, and maybe this doesn't make it less
> confusing... With this patch, I am mainly trying to ensure DevTools uses the
> least privileged principal needed to open the respective page.
> 

(Commenting here before I completely forget my reasoning.) 

This is exactly why I have trouble with the current split.

Based on our APIs, we have three type of links:
- trusted links
- documentation links
- content generated links

When I look at this list, the one I trust the least is "content generated". So I would expect openContentLink to have less privileges than the two others. But right now openDocLink is actually the one that has the less privileges by default.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc6c078e39d9
Centralize DevTools link handling. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/fc6c078e39d9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Haven't been seeing duplicate reports against Fx61 from the wild, so let's let this ride Fx62 to release.
Confirmed with the 62.0b17 build on Win 10x64, macOS 10.13, Ubuntu 16.04LTS.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: