Closed Bug 1297186 Opened 4 years ago Closed 3 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed

People

(Reporter: mstange, Assigned: Nika)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
: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)
See Also: → 1297393
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 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)
(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.
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-
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)
Attachment #8784091 - Attachment is obsolete: true
Attachment #8786828 - Flags: review?(enndeakin) → review+
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
https://hg.mozilla.org/mozilla-central/rev/04010fbe1f10
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.