1707214 - dataTransfer.files is null for drop handler in page, after content script accesses it

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[alexopus.soft]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:88.0) Gecko/20100101 Firefox/88.0

Steps to reproduce:

I want my extension to get a file name from a 'drop' event. For this I add a handler in a content script. Because the page I want this to run on also has a drop handler that calls event.stopPropagation(); event.preventDefault();, my handler has to run in capture mode, to be able to get to the event first.

Here is a code for a minimal reproducer:
https://gist.github.com/alexopus/0444ab296ccdf0c354c2c98413b15942
load the extension, open the html, drop a file onto the drop area.

If I run the same code from the page console instead of an extension, it behaves as I expect.

Am I perhaps missing some permissions in my extension?

Actual results:

The extension successfully captured the file name.
But the drop handler on the page no longer sees the file: Uncaught TypeError: e.dataTransfer.files is null.

Expected results:

Both extension and the page should see the proper e.dataTransfer.files.

Comment 1

[Alex Cornestean]

Hello,

I reproduced the issue on the latest Nightly (90.0a1/20210425214750), Beta (89.0b4/20210425185730) and Release (88.0/20210415204500) under Windows 10 x64 and Ubuntu 16.04 LTS.

As described in the original report, dragging and dropping a file onto the designated area of the .html page will successfully capture the file name but also log Uncaught TypeError: e.dataTransfer.files is null. into the web console. For further details, please see the attached screenshot.

Comment 2

[Alex Cornestean]

Attachment: 2021-04-26_10h54_17.png

Comment 3

[alexopus.soft]

Some update/more details:
It looks like the the breakage happens if the extension accesses the dataTransfer.files before the page does.
I did another test where I modified the given extension code, and wrapped access to dataTransfer.files with a setTimeout(..., 500); This way page would (likely) access it before the extension. Furthermore, even if page accesses it again at a later time (e.g. one more time after 1000ms), it still works.

So it looks like the bug only occurs if the extension accesses the object first.
Here is another gist with a "working" version:
https://gist.github.com/alexopus/ee3141065cfba6da75ebe1e1c0b279fa

This workaround is still fragile though, since I have no way of telling when the page has accessed dataTransfer.files (that I am aware of).

Comment 4

[Alex Cornestean]

Hello,

Retested the issue with the new test add-on/test page (Comment 3) on the latest Nightly (90.0a1/20210426213158), Beta (89.0b4/20210425185730) and Release (88.0/20210415204500) under Windows 10 x64 and Ubuntu 16.04 LTS.

This time around, proceeding with the steps to reproduce, the error is no longer logged into the web console. So it appears the changes described in Comment 3 solve the issue. For further details, please see the attached screenshot.

Comment 5

[Alex Cornestean]

Attachment: 2021-04-27_12h40_40.png

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1707214 - Ensure we don't cache dataTransfer.files when accessed by expanded principals. r?nika!,mixedpuppy

This doesn't seem a recent regression, the underlying issue is that we don't account for
the Extensions content scripts (and user scripts) as other principals that may be able
to intercept a drop event and then try to access the dataTransfer.files property before
the webpage does.

As the big inline comment inside DataTransferItemList::Files does explain, keeping a copy
of the FileList when accessed from a webpage principal is necessary for spec compliance,
while we don't cache it when it is being accessed by system principal code.
(see https://searchfox.org/mozilla-central/rev/6cbe34b441f7c7c29cd1e5f0e19c7000142f1423/dom/events/DataTransferItemList.cpp#200-242)

The changes in this patch are preventing us from caching the file list also when accessed
by expanded principals (which are never going to be used for any web page, but they are used
by both Extensions content scripts and Extensions user scripts), along with adding a new.
regression test to prevent it from regressing without being noticed.

Without this patch, when an extension content script or user script does intercept the
drop event and access the dataTransfer.files property, we cache the FileList created
for the ExpandedPrincipal associated to the Extension script and after that if the page
try to access it we do return null because the webpage principal doesn't subsume the
expanded principal (which is also why the issue isn't triggered if the webpage does
access it first). In debug build we crash because we hit the assertion in
DataTransferItemList::Files right before the earlier nullptr return we hit on release.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cae6a227bde7d681163d30a56c1aa8c3c811191c

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4ca0437aa3f
Ensure we don't cache dataTransfer.files when accessed by expanded principals. r=mixedpuppy,nika

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b4ca0437aa3f

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:rpl, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Alex Cornestean]

Hello,

I verified the fix using the original test add-on on the latest Nightly (90.0a1/20210503214210) under Windows 10 x64 and Ubuntu 16.04 LTS.

The issue is fixed, as both the extension and the page see the transfer. The previous error related to this is not logged in the web console. For further details, please see the attached screenshot.

Comment 12

[Alex Cornestean]

Attachment: 2021-05-04_12h57_37.png

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #10)

The patch landed in nightly and beta is affected.
:rpl, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Given that this wasn't a recent regression (based on a brief look to the hg annotate info for the related code in DataTransferItemList, it looks this may have been the case at least starting from Firefox 50) and we didn't got bug reports about this issue before, it seems that not many extensions are actually hitting this and so personally I think it may be reasonable to let this just ride the train.