1146116 - Files set by mozSetFileArray inaccessible to page

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: test-frame.js

To get around bug 1143934, I switched to using mozSetFileArray, which works as intended, except that when I look at the File using the devtools, it's just "Object", with no accessible properties.


STR:
Load a form with a file input control.
Load the attached frame script.
Call messageManager.sendAsyncMessage("setfiles", new File("/path/to/a/file")). The value of the control should change. Check the control's files property in developer tools.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Call messageManager.sendAsyncMessage("setfiles", new File("/path/to/a/file"))

Call where?  This is creating a File in a system-principal context, I assume?

If so, then after that it's placed in a page <input>, where the C++ code can touch it (though arguably we should fix that, because it would provide much more actionable feedback than what happens now) but the JS object visible to the web page is a COW with, as you note, no accessible properties.

The right way to use mozSetFileArray with Files you are creating is to create the File objects using |new content.File|, if you're going to put them into some element residing in content.document.

If that's not possible for some reason, we _could_ consider having mozSetFileArray clone the File instances as needed, but we'd have to audit existing consumers to make sure that doesn't break them...

Comment 2

[Geoff Lankow (:darktrojan)]

(In reply to Not doing reviews right now from comment #1)
> The right way to use mozSetFileArray with Files you are creating is to
> create the File objects using |new content.File|, if you're going to put
> them into some element residing in content.document.

I can't create a File from the filesystem, as it isn't allowed on the content side.
Passing a File/Blob/TypedArray via the messageManager and creating a File from it doesn't work (I assume it's for a security reason).


Why does this test work? Is it only because the input element is wrapped?
https://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug403852.html?force=1

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Not doing reviews right now from comment #1)
> > Call messageManager.sendAsyncMessage("setfiles", new File("/path/to/a/file"))
> 
> Call where?  This is creating a File in a system-principal context, I assume?

Call that in the system-principal context of a chrome script; the message is received (and the File is reconstituted) in the system-principal context of a frame script.  So, "yes".

The frame script would need to use Cu.cloneInto on the received file before passing it to mozSetFileArray, I think.

> If that's not possible for some reason, we _could_ consider having
> mozSetFileArray clone the File instances as needed, but we'd have to audit
> existing consumers to make sure that doesn't break them...

mozSetFileArray is relatively new — it was added in bug 1068838 for use by mochitests, which see the File objects through permissive COWs and therefore didn't need to care about its wrapperness.  (mozSetFileArray is also not really documented, yet.)

Also, for what it's worth: SessionStore is affected by bug 1143934 and is going to run into the same problems as add-ons here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I can't create a File from the filesystem, as it isn't allowed on the content side.

Ah.  So the issue is that you're creating the File in the chrome process, then passing it to the content process (structured cloning in the process)?

You _should_ be able to do this in the content process:

  var contentExposedFile = new content.File(myChromeFile, { name: myChromeFile.name });

but I agree that this is a bit of a pain.

> and creating a File from it doesn't work (I assume it's for a security reason)

Creating how?  The File constructor does only allow doing new File(someOtherFile) for chrome code, but I'd think your frame script is chrome code, right?

> Why does this test work?

Because, as Jed mentioned, SpecialPowers cheats.  From specialpowersAPI.js:

31 // Allow stuff from this scope to be accessed from non-privileged scopes. This
32 // would crash if used outside of automation.
33 Cu.forcePermissiveCOWs();

so we get transparent wrappers instead of opaque ones.

> The frame script would need to use Cu.cloneInto on the received file before passing it
> to mozSetFileArray

Yes, that would work.  That's cleaner than using the File constructors, for sure.

That said, it sounds like mozSetFileNameArray might be something we want to restrict to test automation....

Comment 5

[Geoff Lankow (:darktrojan)]

(In reply to Not doing reviews right now from comment #4)
> You _should_ be able to do this in the content process:
> 
>   var contentExposedFile = new content.File(myChromeFile, { name:
> myChromeFile.name });
> 
> but I agree that this is a bit of a pain.

That's what I tried and it throws NS_ERROR_FAILURE.

> > and creating a File from it doesn't work (I assume it's for a security reason)
> 
> Creating how?  The File constructor does only allow doing new
> File(someOtherFile) for chrome code, but I'd think your frame script is
> chrome code, right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> That's what I tried and it throws NS_ERROR_FAILURE.

Ah, because the security check is in the ctor impl, at which point we've already dropped privileges.  So yeah, Cu.cloneInto is what you want.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Before mozSetFileArray was added, if I understand correctly, there was no way for an HTMLInputElement to hold a cross-origin File — mozSetFileNameArray, or setting |value|, or using a file picker, would all create a File belonging to the input element's global.

Breaking that invariant with mozSetFileArray wasn't intentional.  Would there be objections to restoring it?

Comment 8

[Boris Zbarsky [:bzbarsky]]

The main reason we don't automatically do that sort of thing normally is to avoid accidentally leaking things to pages.

But given that this API exists for the specific reason of exposing the files, I think it'd be fine if it cloned the files into the page global as needed.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1146116-setfile-clone-hg0.diff

The C++ change is small; most of the patch is adding a regression test.  I've verified that the test fails without the rest of the patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8583343 [details] [diff] [review]
bug1146116-setfile-clone-hg0.diff

You should probably bail out if GetScopeObject() returns null...

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8583343 [details] [diff] [review]
bug1146116-setfile-clone-hg0.diff

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

::: dom/html/test/test_bug1146116.html
@@ +23,5 @@
> +const xhr = new XMLHttpRequest;
> +xhr.open("GET", "/dynamic/getMyDirectory.sjs", false);
> +xhr.send();
> +const basePath = xhr.responseText;
> +const here = basePath + "/test_bug1146116.html"

…which doesn't work for mochitest-remote (B2G and Fennec), because the test files aren't local.

(Yes, I asked for review before the try run of the final version of the patch had finished.  Mea culpa.)

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch [v2]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=847f6ef25e01

The test is a little different now — it creates an empty temporary file and removes it afterwards.  (This will be useful for at least one other test, later, in bug 1143934.)

I'm not entirely sure if my check for GetScopeObject returning null is the right level of “can't happen”, but other uses seem to either MOZ_ASSERT or just not check at all, so it seems reasonable.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8584570 [details] [diff] [review]
Patch [v2]

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

::: dom/html/HTMLInputElement.cpp
@@ +2332,5 @@
>  void
>  HTMLInputElement::MozSetFileArray(const Sequence<OwningNonNull<File>>& aFiles)
>  {
> +  nsCOMPtr<nsIGlobalObject> global = OwnerDoc()->GetScopeObject();
> +  MOZ_ASSERT(global);

I don't actually know when GetScopeObject might return null. But it's probably safe to say that we shouldn't call mozSetFileArray when that's the case so this assertion is probably fine.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

See comment #12 for try run.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fff872add31

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8fff872add31

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8584570 [details] [diff] [review]
Patch [v2]

Approval Request Comment
[Feature/regressing bug #]: Bug 1068838
[User impact if declined]: Increased annoyance for add-on creators — if this is uplifted then mozSetFileArray will behave the same way in all versions where the property exists, instead of 38 needing special workarounds.  (See comment #0 for an example of what happens without this patch.)
[Describe test coverage new/current, TreeHerder]: Patch contains a test case; it's been baking on m-c for a few days.  Several other tests use mozSetFileArray to test forms.
[Risks and why]: Low; we're not using this in our code (outside of tests) yet, but it can be used by add-ons that manipulate HTML forms and want to be e10s-ready.[String/UUID change made/needed]: None.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8584570 [details] [diff] [review]
Patch [v2]

should be part of 38 beta 2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d3e9b16fc53f