Closed Bug 1303727 Opened 8 years ago Closed 7 years ago

[e10s] Local files downloaded in the /Temp folder and opened in read-only mode

Categories

(Firefox :: File Handling, defect, P5)

Product:
Firefox
Component:
File Handling
Version:
48 Branch
Platform:
Unspecified
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
e10s - ---
firefox52 --- verified
firefox53 --- verified

People

(Reporter: bart, Assigned: mrbkap)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1303727 - Propagate whether we're dealing with a local file to the parent.
(deleted), text/x-review-board-request
jduell.mcbugs
: review+
jcristau
: approval-mozilla-aurora+
Details 
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606100503

Steps to reproduce:

1. Make link with local file <a href="file://N:/path/to/file.ext">
2. Click
3. Choose "Open with"


Actual results:

File is copied locally and opened read-only.


Expected results:

File should be opened from local path.
I have these user.js settings:

user_pref("capability.policy.policynames", "localfilelinks");
user_pref("capability.policy.localfilelinks.sites", "http://my.intranet");
user_pref("capability.policy.localfilelinks.checkloaduri.enabled", "allAccess");

Firefox 48 on Windows 7 and 10 have the issue, Firefox 47 on Linux does not.
OS: Unspecified → Windows
It's an issue only with e10s enabled.

STR for local PDF:
firefox:
about:preferences#applications > PDF: Always Ask
html:
<a href="file://C:/testcase/a.pdf">Link</a>
user.js:
user_pref("capability.policy.policynames", "localfilelinks");
user_pref("capability.policy.localfilelinks.sites", "http://localhost");
user_pref("capability.policy.localfilelinks.checkloaduri.enabled", "allAccess");

When opening the file with my 3rd-party PDF reader, the local file is downloaded in the /temp folder instead of being opened from its local path.
tracking-e10s: --- → ?
Component: Untriaged → File Handling
Summary: local files copied before opening → [e10s] Local files copied before opening
Summary: [e10s] Local files copied before opening → [e10s] Local files downloaded in the /Temp folder before opening
Blocks: e10s
This is indeed because of e10s, when I disable that by setting "browser.tabs.remote.autostart.2" to "false", it works as expected.
@ Neil,
How can you mark bug 1302553 as a duplicate of this Bug, when 1302553 was reported 6 Days earlier? I would assume, that it is the other way round.
(In reply to David Weidlinger from comment #5)
> @ Neil,
> How can you mark bug 1302553 as a duplicate of this Bug, when 1302553 was
> reported 6 Days earlier? I would assume, that it is the other way round.

Because this bug has better STR, duplicating bugs doesn't follow chronology every time.
Ok.the str are complete in 1302553 (compute working and non working config Attachment and a filepath example).
It was even mentioned, that it is a bug in e10s v1.2 and v1.1 is working correct.
But you are right. This Bug of course has a better str. :)
I am just happy with that, when it is fixed.
I can reproduce this bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Only if you have e10s v1.2 installed.
If u got 1.0 or 1.1 you won't be able to reproduce i guess.
You would be able to read this in bug 1302553 already. But now it is w ritten her too and so it is ok.
Just wondering about the immediately answers in here containing the informations, that i already added to 1302553, while i was waiting for one week and did not get any reactions.
I wrote the reproductionprocess beginning at a clean new profile. You could not make a reproductionprocess more robust than that.
I am just a little **** because I need that very urgent for my work and nobody answered and here the first answer did take 2 hours. And i even wenn to the IRC chat to report. 
But I stop that now. This is not a forum. Sorry.
Now in this bug the Information is complete as well, like it was in the original bugreport already one week ago.
The simplest workaround is to disable e10s in your profile.
Who is the right person to decide whether we're going to fix this?
Flags: needinfo?(enndeakin)
Keywords: regression
I really don't know.
Flags: needinfo?(enndeakin)
User impact: User cannot edit files after user opens a file via Firefox.
Suggest to fix this on next release. Set priority to P2. 
Please change the priority to P3 if you think it's not a critical problem.

@ Ben, this is a feature regression.
May I have your help to escalate this issue to the right person?
Thank you.
Flags: needinfo?(btian)
Priority: -- → P2
Paolo,

Can you help decide whether to fix this bug (comment 11) or forward this bug to e10s people?
Flags: needinfo?(btian) → needinfo?(paolo.mozmail)
As I understand it, and please correct me if I'm wrong, the use case that Bart and David require is that they want to deploy a web page through an Intranet site, and that page would contain links to local files that would be launched directly on the client computer.

As far as I remember, we don't officially support this use case and the associated "about:config" preferences in Firefox, and I'm actually surprised it works as described without an add-on. Going forward, I think this use case is best handled by deploying an add-on to the clients that need this functionality. This is also more robust and visible than deploying custom "about:config" preferences.

Bart, David, for the moment your workaround of disabling e10s should work, but you should look into migrating your use case to an add-on. The closest existing add-on I could find is this one:

https://addons.mozilla.org/firefox/addon/local-filesystem-links/

This doesn't open the file directly, it only shows it in the file manager, but you may be able to get in touch with the author or adapt it to your specific needs, limiting its functionality to your Intranet site for example.
Flags: needinfo?(paolo.mozmail)
Priority: P2 → P5
@Paolo,

This feature has worked since ever and in my opinion, it should work. Also, according to this page: http://kb.mozillazine.org/Firefox_:_Issues_:_Links_to_Local_Pages_Don't_Work it should work when the correct preferences are set, for security (and not functional) reasons. This is quite an old page and I'd say that kind of tells me it is supported.
There are two aspects to this issue, the security preferences and the behavior once the link is clicked.

I would say that the security preferences still work as documented on the page you mentioned. They are maintained by a different team and as far as I can tell there have been no regressions, because you can still load local links with e10s once you set the preferences.

The other aspect is the front-end behavior. We are reworking how the downloads front-end works in fundamental ways, and this is tracked in meta-bug 1268398. In doing this, we are not keeping the Intranet direct link use case into account in the core product. You can disagree, but we have to keep our core product focused on what gives the greatest benefit to the majority of users.

If this bug turns out to be an easy fix and someone comes up with a good patch we may consider taking it, but the use case will likely break again in the future. Having an add-on tailored to it would probably serve your needs better.
Thanks for the clearing up. We'll see how it pans out and if the behaviour is not going into the core product, I will have to resort to (or write) an add-on.
French users reported the same issue:
https://forums.mozfr.org/viewtopic.php?f=5&t=131192

IMO, we should reconsider the priority of this regression because it breaks a functionality used since many years (some users used it as personal access to local files).
Summary: [e10s] Local files downloaded in the /Temp folder before opening → [e10s] Local files downloaded in the /Temp folder and opened in read-only mode
We should reconsider the importance of this regression when e10s is going to be enabled for all the users in 2017, considering the amount of dupes and potential users affected in enterprise (with intranet documents directly loaded in a webpage).
Flags: needinfo?(lhenry)
Paolo's answer makes sense to me, too. What we might want to do is communicate about the change and possible workarounds. Jimm, what do you think? It's a good point that this already has duplicates, and we'll likely see more in the next few months.
Flags: needinfo?(lhenry) → needinfo?(jmathies)
Not sure where this handling decision is getting made, but I'm guessing this is tied to the way necko handles file downloads with e10s. Also though this may tie into sandboxing work since we just landed a local file content process, which should handle these downloads.

Jason, can you enlighten us as to how the networking code handles this situation?
Flags: needinfo?(jmathies) → needinfo?(jduell.mcbugs)
Flags: needinfo?(bobowencode)
I rememeber recently the e10s team wrote some code that now launches top-level file:// loads into a new process (so normal content processes can keep file sandboxing). I wonder if this copy-to-tmp behavior was part of that?  But I can't find the bug anywhere.  I'm hoping Chris Peterson knows.
Flags: needinfo?(cpeterson)
I think Bob Owen (needinfo'd) will know about temp file handling within the sandbox.
Flags: needinfo?(cpeterson)
(In reply to Jason Duell [:jduell] (needinfo me) from comment #25)
> I rememeber recently the e10s team wrote some code that now launches
> top-level file:// loads into a new process (so normal content processes can
> keep file sandboxing). I wonder if this copy-to-tmp behavior was part of
> that?  But I can't find the bug anywhere.  I'm hoping Chris Peterson knows.

That only landed in Fx53 (bug 1147911).
This looks like it is an e10s thing given that it was reported against Fx48.

Isn't the problem that it is treating the file:// resource like a web resource (which has to be copied locally to open it) and it didn't do that before.
Flags: needinfo?(bobowencode)
I've looked over the necko file protocol code, just in case it was the issue (unlikely, since we've never modified it for e10s) and indeed, I don't see any logic that copies the file before we read it.

> Isn't the problem that it is treating the file:// resource like a web resource 
> (which has to be copied locally to open it) 

Could you say more about this?  If by 'web resource' you mean http{s}:// URIs, for those we open the actual TCP socket in the parent process, then send the data (via IPDL) to the child.  But nsFileChannel is not doing that.  We seem to be actually coping the file to /tmp before we open it.  And I don't know where the code that does that lives.
Flags: needinfo?(bobowencode)
(In reply to Jason Duell [:jduell] (needinfo me) from comment #29)
> I've looked over the necko file protocol code, just in case it was the issue
> (unlikely, since we've never modified it for e10s) and indeed, I don't see
> any logic that copies the file before we read it.
> 
> > Isn't the problem that it is treating the file:// resource like a web resource 
> > (which has to be copied locally to open it) 
> 
> Could you say more about this?  If by 'web resource' you mean http{s}://
> URIs, for those we open the actual TCP socket in the parent process, then
> send the data (via IPDL) to the child.  But nsFileChannel is not doing that.
> We seem to be actually coping the file to /tmp before we open it.  And I
> don't know where the code that does that lives.

I honestly don't know what code does that, I was just trying to interpret the original post.
As in it seems like with e10s, file:// URIs opened externally in this way are being treated in the same way as http(s):// ones and they weren't before.
Flags: needinfo?(bobowencode)
Given that this only affects opening a file:// URI with an external app (see comment 0) I'm guessing that this has nothing to do with the nsFileChannel, etc code, and is instead something that's happening with the code that orchestrates external app launch.  I thought that might be the code in uriloader/exthandler, but I haven't found it in there either.

Checking to see if Blake might know where the code lives.
Flags: needinfo?(mrbkap)
At a high level, here's what I'm guessing our options for this bug are:

1) Do nothing and break these users.  But there seem to be a fair number of them, and the fix probably isn't technically difficult.

2) Change the code that copies to /temp to first check whether the prefs in comment 1 are set.  If localfilelinks.sites matches the page that's being loaded, then don't copy the file, and instead open it directly.  This would presumably require that file sandboxing be turned off, which is obviously a security loss, but I'm guessing many of these users would choose that over having this feature disappear.

3) If we don't want to turn off file sandboxing automagically, then we could require people who still want this feature to manually set yet another pref where they explicitly opt-into disabling file sandboxing.  That might be a fair tradeoff.

(I don't know much about sandboxing, but I'm assuming that launching a new process that has read/write access to a file isn't something that a sandboxed process is allowed to do.)
This is almost certainly related to nsExternalHelperAppService. I'll take a look next week to see how hard it would be to fix this. As far as I know, this wasn't an intentional change by the e10s team; rather, it's an unintended side-effect on a relatively rare use-case. It should also be completely orthogonal to the new sandboxing stuff, as (AIUI) we should still be loading these links in an external application instead of showing them directly in Firefox itself.
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8825609 [details]
Bug 1303727 - Propagate whether we're dealing with a local file to the parent.

https://reviewboard.mozilla.org/r/103724/#review104530

::: uriloader/exthandler/ExternalHelperAppParent.h:31
(Diff revision 1)
>  class PChannelDiverterParent;
>  } // namespace net
>  
>  namespace dom {
>  
> +#define NS_IEXTERNALHELPEAPPPARENT_IID \

Drive-by: there's a typo here.

Thanks for looking into this!
(In reply to :Paolo Amadini from comment #35)
> Drive-by: there's a typo here.

I've fixed this locally. Thanks for pointing it out.
Comment on attachment 8825609 [details]
Bug 1303727 - Propagate whether we're dealing with a local file to the parent.

https://reviewboard.mozilla.org/r/103724/#review106598

Looks good.
Attachment #8825609 - Flags: review?(jduell.mcbugs) → review+
Blake,

I'll let you upload the patch with the typo fix.  I'm assuming we want to uplift this as much as possible.
Flags: needinfo?(mrbkap)
Comment on attachment 8825609 [details]
Bug 1303727 - Propagate whether we're dealing with a local file to the parent.

Approval Request Comment
[Feature/Bug causing the regression]: e10s
[User impact if declined]: Opening local files through Firefox in external applications will open a readonly copy instead of the version already on the local machine.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 2.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The change only affects this specific case and doesn't have interactions with many other parts of the code.
[String changes made/needed]:
Flags: needinfo?(mrbkap)
Attachment #8825609 - Flags: approval-mozilla-aurora?
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/367612b00df0
Propagate whether we're dealing with a local file to the parent. r=jduell
https://hg.mozilla.org/mozilla-central/rev/367612b00df0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment on attachment 8825609 [details]
Bug 1303727 - Propagate whether we're dealing with a local file to the parent.

fix opening files in other apps on mac, aurora52+
Attachment #8825609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It would be good to have verification of this fix in nightly (and aurora when it lands), see comment 2.
Flags: qe-verify+
I have reproduced this bug with Firefox nightly 49.0a1(build id:20160606100503)on
windows 7(64 bit)

Verified this bug as fixed with Firefox aurora 52.0a2(build id:20170119004006)
user agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

Verified this bug as fixed with Firefox nightly 53.0a1(build id:20170120030214)
user agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Setting status to verified per comment 46.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: