Crash when dragging file over file input, in nsFileControlFrame::DnDListener::CanDropTheseFiles(nsIDOMDataTransfer*, bool)

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mstange, Assigned: mystor)

Tracking

({crash})

Trunk
mozilla51
crash
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-d2018f19-aba7-4164-bcac-365032160822.
=============================================================

Steps to reproduce:
1. Click on "Attach file" in this bug.
2. Open Finder, pick a file, and drag it over the "Browse..." button.

Crash.
Hi Michael, is this related to bug 1296041? Any more ideas? Thanks!
Flags: needinfo?(michael)
A bit different STR:
- win10
- open 'add an attachment' page in bugzilla
- drop a file on the [ Browse ] button (in my case a txt file)

I believe it started with https://hg.mozilla.org/mozilla-central/rev/194fe275b4e60ded2af6b25173eec421f0dba8ad
(Assignee)

Comment 3

a year ago
:mstange contacted me about this tomorrow, but I wasn't able to work on it as I was out of the office. I've got a pretty good idea of what's going on, so it shouldn't be too bad to fix.
Assignee: nobody → michael
Flags: needinfo?(michael)
(Assignee)

Updated

a year ago
See Also: → bug 1297393
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7d3bc3efebe0730a8f8d560ca5c07eb2501d74ad&tochange=96766f088fd33489b6b8011cc685eb09fabb0d19

Also reproducible on Windows.
Blocks: 1296041
status-firefox50: --- → unaffected
OS: Mac OS X → All
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
Comment on attachment 8784091 [details]
Bug 1297186 - Correctly propagate principals when getting the files list in DataTransfer,

Adding Neil as a reviewer to see if we can speed up getting this patch landed.
Attachment #8784091 - Flags: review?(enndeakin)

Comment 8

a year ago
mozreview-review
Comment on attachment 8784091 [details]
Bug 1297186 - Correctly propagate principals when getting the files list in DataTransfer,

https://reviewboard.mozilla.org/r/73654/#review72848

::: dom/events/DataTransferItemList.cpp:208
(Diff revision 2)
> -FileList*
> -DataTransferItemList::Files()
> +already_AddRefed<FileList>
> +DataTransferItemList::Files(nsIPrincipal* aPrincipal)
>  {
> +  RefPtr<FileList> files;
> +  if (nsContentUtils::IsSystemPrincipal(aPrincipal)) {
> +    NS_WARNING("Accessing files from System Principal");

Why is there a warning here?

::: dom/events/DataTransferItemList.cpp:220
(Diff revision 2)
>      mFiles = new FileList(static_cast<nsIDOMDataTransfer*>(mDataTransfer));
> +    mFilesPrincipal = aPrincipal;
>      RegenerateFiles();
>    }
>  
> -  return mFiles;
> +  if (!aPrincipal->Subsumes(mFilesPrincipal)) {

This is a strange setup and needs more comments in the code.

Does it work if a chrome caller accesses dataTransfer.files then a caller on the content page later does so?

::: dom/events/DataTransferItemList.cpp:302
(Diff revision 2)
>                                             nsIPrincipal* aPrincipal,
>                                             bool aInsertOnly,
>                                             bool aHidden,
>                                             ErrorResult& aRv)
>  {
> +  if (!nsContentUtils::IsSystemPrincipal(aPrincipal)) {

This is called during FillInExternalData where these checks should not be performed.
Attachment #8784091 - Flags: review?(enndeakin)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
(In reply to Neil Deakin from comment #8)
> Comment on attachment 8784091 [details]
> Bug 1297186 - Correctly propagate principals when getting the files list in
> DataTransfer,
> 
> https://reviewboard.mozilla.org/r/73654/#review72848
> 
> ::: dom/events/DataTransferItemList.cpp:208
> (Diff revision 2)
> > -FileList*
> > -DataTransferItemList::Files()
> > +already_AddRefed<FileList>
> > +DataTransferItemList::Files(nsIPrincipal* aPrincipal)
> >  {
> > +  RefPtr<FileList> files;
> > +  if (nsContentUtils::IsSystemPrincipal(aPrincipal)) {
> > +    NS_WARNING("Accessing files from System Principal");
> 
> Why is there a warning here?

Accidental leftover from debugging. Removed.

> ::: dom/events/DataTransferItemList.cpp:220
> (Diff revision 2)
> >      mFiles = new FileList(static_cast<nsIDOMDataTransfer*>(mDataTransfer));
> > +    mFilesPrincipal = aPrincipal;
> >      RegenerateFiles();
> >    }
> >  
> > -  return mFiles;
> > +  if (!aPrincipal->Subsumes(mFilesPrincipal)) {
> 
> This is a strange setup and needs more comments in the code.
> 
> Does it work if a chrome caller accesses dataTransfer.files then a caller on
> the content page later does so?

Yes, I added some comments.

> ::: dom/events/DataTransferItemList.cpp:302
> (Diff revision 2)
> >                                             nsIPrincipal* aPrincipal,
> >                                             bool aInsertOnly,
> >                                             bool aHidden,
> >                                             ErrorResult& aRv)
> >  {
> > +  if (!nsContentUtils::IsSystemPrincipal(aPrincipal)) {
> 
> This is called during FillInExternalData where these checks should not be
> performed.

I don't think it is. FillInExternalData is defined on the DataTransferItem now, and as far as I know never calls into DataTransferItemList::SetDataWithPrincipal anymore.

If you're talking about CacheExternalData (which adds the placeholder DataTransferItem instances), that method is always called with the system principal as aPrincipal, so it shouldn't be an issue as far as I know.
(Assignee)

Updated

a year ago
Attachment #8784091 - Flags: review?(amarchesini) → review?(enndeakin)
The patch seems ok, but when I drag an image to the desktop on Mac I get a file shortcut instead of the real image. The filename is something like file-///directory/happy.png.fileloc rather than just the filename.

The data appears to be missing the file promise type (as well as application/x-moz-nativeimage)
Attachment #8784091 - Flags: review?(enndeakin) → review-
(Assignee)

Comment 12

a year ago
Created attachment 8786828 [details] [diff] [review]
Correctly propagate principals when getting the files list in DataTransfer

This patch moves the check out of SetDataWithPrincipal, and instead explicitly checks at the call sites which need to check.
Attachment #8786828 - Flags: review?(enndeakin)
(Assignee)

Updated

a year ago
Attachment #8784091 - Attachment is obsolete: true
Attachment #8786828 - Flags: review?(enndeakin) → review+

Comment 13

a year ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04010fbe1f10
Correctly propagate principals when getting the files list in DataTransfer, r=enndeakin

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04010fbe1f10
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.