1360441 - Copy a bigger amount of data fails

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect, P3)

Description

[michael.gras]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170418123315

Steps to reproduce:

Run the following code.
https://jsfiddle.net/m951utk7/4/

It uses a library called Clipboard.js to copy data to the clipboard. This one copies the generated string to a hidden textarea and then calls document.execCommand("copy").

//////////////////////////////////
// Custom code to reproduce
// Include Clipboard.js (by https://zenorocha.github.io/clipboard.js)
https://cdnjs.cloudflare.com/ajax/libs/clipboard.js/1.6.1/clipboard.js

// HTML
<button class="btn">Copy</button>

// Jacascript
var clipboard = new Clipboard('.btn', {
	text: function(trigger) {
  	let myString = "";
    for (let i=0; i != 100000; ++i) {
    	myString += "I am a very long text\n";
    }
    window.alert("Created text of length " + myString.length);
  	return myString;
    }
});

clipboard.on('success', function(e) {
  window.alert("Success");
});

clipboard.on('error', function(e) {
  window.alert("Error");
});

// press copy button


Actual results:

The programm freezes for some like 10 seconds (since copying to the textarea is really slow) and afterwards an error message is displayed, as  document.execCommand throws an exception. 

In the Browser console the following message is written:
document.execCommand(‘cut’/‘copy’) was denied because it was not called from inside a short running user-generated event handler.


Expected results:

I expected that the generated string is copied to the clipboard like in other browsers. Furthermore, I expected the whole process of copying a string of 3MB to a textarea to be much faster.

Chrome 58 (works fine)
Firefox 53 (does not work)
Opera 44 (works fine)
Internet Explorer (Windows 7) (works fine)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hmmm, it failed even if I created a very short text with 22 characters for testing...

Hey Michael, would you be able to take a look at this? Thanks!

Comment 2

[michael.gras]

Hey Hsin-Yi,

I update the code by commenting the alert message. 
https://jsfiddle.net/m951utk7/8/

Displaying the alert message raises the same message in the console:
document.execCommand(‘cut’/‘copy’) was denied because it was not called from inside a short running user-generated event handler.

Best regards
Michael

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Our code for determining if an event is currently being run in response to a user-generated action has a timeout associated with it which is configured by the pref "dom.event.handling-user-input-time-limit". I believe that by default it is 1 second. This was added in bug 684627. I'm not sure exactly why we have this timeout, and Ehsan was quite surprised when I told him that it existed. I've ni?-ed olli for his input on this time limit.

In the example jsfiddle you provided, the alert dialog pops up, and stays open for long enough that the copy then fails. On my computer, when this alert is removed, the code no longer fails to copy the text to the clipboard. This alert box is probably the cause of the failure which Hsin-Yi was running into with small character counts.

There is still a serious hang which is caused by the script while setting up the textarea to copy into. I'm inclined to transform this bug into a bug about the hang which we have in this situation.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Not sure what is being asked here.
I think https://bugzilla.mozilla.org/show_bug.cgi?id=684627#c0 is quite clear.

Comment 5

[(no longer active)]

Olli, do you think that it is desirable that Michael's test case would work on a fast computer and would break on a slow computer, depending on whether Gecko can finish reflowing the page before the 1 second time limit fires?

I find that unacceptable.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Not sure. I consider it also a security feature. One may trick user to do something odd by slowing down processing. IsHandlingUserInput is after all a security feature.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

The hang part of this bug is caused by the expensive reflow which is triggered when the newly added textarea is synchronously reflowed due to it being selected. From my local testing, it seems as though this is also terrible in chrome, however they don't have the same timeout on their IsHandlingUserInput equivalent, so this problem doesn't come up.

The easiest solution for us right now would be to relax or remove this timer.

With my small testing program, and on my machine (ymmv), Firefox performs this synchronous reflow and successfully copies the text to the clipboard in ~400ms, while chrome does it in ~1000ms. On my laptop Firefox takes ~1500ms (and fails to copy because it took too long), chrome takes ~500ms, and safari takes ~3300ms.

I also wrote another version of this function for copying text to the clipboard to instead fill the clipboard using the "copy" clipboard event, rather than creating the DOM node, selecting & copying it, and then removing it, with code which looks like:

  // Register a copy handler which will directly add the text to the DataTransfer object.
  function copyHandler(event) {
    event.clipboardData.setData("text/plain", text);
    event.preventDefault();
  }
  document.addEventListener("copy", copyHandler);
  // Trigger the copy command, and then remove the event listener
  document.execCommand("copy");
  document.removeEventListener("copy", copyHandler);

This completes in around ~20ms on Firefox and ~5ms on Chrome on my computer.

Unfortunately, we can't just suggest that Clipboard.js changes to using this, as according to caniuse, on safari and internet explorer, there must be a valid selection for the copy event to fire. I wrote another version which generates a textarea offscreen with an "a" in it, and uses this selection, and it appears to work in safari, IE and Edge. 

I've posted my test HTML file here: https://mystor.github.io/copy-reflow/

I'll probably make a PR to Clipboard.js and see if they want to take it, because based on the fact that every browser seems to hang when trying to copy this much data, I'm inclined to think we can't fix the sync reflow issue.

Comment 8

[michael.gras]

Hi there,
thank you for your investigation. I hope that you can improve on the performance synchronous reflow in the future. For now, I just updated the case, that I previously created for Clipboard.js, and posted your "workaround". Let's see how they react.
https://github.com/zenorocha/clipboard.js/issues/407

Best regards
Michael

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Not sure. I consider it also a security feature. One may trick user to do
> something odd by slowing down processing. IsHandlingUserInput is after all a
> security feature.

I'm inclined to, at the very least, increase or remove this timeout for the copy callback (even if we don't increase it for the fullscreen callback, which has a worse failure case). This wouldn't fix the hang issue (which from my testing seems to occur in every browser, many worse than Firefox), but would at the very least avoid breaking this code.

The hang issue might get fixed in clipboard.js with my PR (https://github.com/zenorocha/clipboard.js/pull/412), but there will still be websites using the older version of the code.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I'd prefer if some security folks could say their opinion. dveditz perhaps?

Comment 11

[Nika Layzell [:nika] (ni? for response)]

dveditz, please see comment 9 and 10.

Comment 12

[(no longer active)]

This isn't a security feature, it's an anti-spoofing feature designed for fullscreen initially.  Even if this is a good idea for fullscreen, the reason is that the possible attack is about overlaying a fullscreen window imitating the browser/OS UI.  That just doesn't even apply to execCommand.

I suggest regardless of the answer to comment 11, we should make execCommand only obey the stack based check and not the time based check.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Disable the IsHandlingUserInput timeout for execCommand(copy/cut) commands

MozReview-Commit-ID: 341K1DEsVCg

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #12)
> This isn't a security feature, it's an anti-spoofing feature designed for
> fullscreen initially.

Was it? I thought it was even earlier, for the popup blocker. We wanted the result to be in some reasonable proximity to the user action so the user would associate the two, not have popups show up 30 seconds after you manage to convince the user to click on something. "The _site_ did this in response to what I was doing" rather than "my Firefox is possessed!".

Looking at that bug I guess you are right: it was fullscreen. The popup-blocker was just looking for "inside an event" and didn't consider the possibility of arbitrarily long-lived event handlers.

> That just doesn't even apply to execCommand.

I agree. If the site doesn't make it clear that a particular site element is doing a copy to clipboard then no matter what kind of time-limit we impose the user experience would be that their clipboard was mysteriously stomped on by something, almost certainly discovered long after the event that did it.

It's still good to require it be in an event handler so some background page can't stomp your clipboard in a loop (see the eviltraps bug for a long list of pointless abusive things jerks do just because they can), but the time-limit isn't helping here.

Comment 15

[(no longer active)]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #12)
> > This isn't a security feature, it's an anti-spoofing feature designed for
> > fullscreen initially.
> 
> Was it? I thought it was even earlier, for the popup blocker.

See comment 4.

> We wanted the
> result to be in some reasonable proximity to the user action so the user
> would associate the two, not have popups show up 30 seconds after you manage
> to convince the user to click on something. "The _site_ did this in response
> to what I was doing" rather than "my Firefox is possessed!".

That can't really happen given that we have detection for slow running scripts.

> Looking at that bug I guess you are right: it was fullscreen. The
> popup-blocker was just looking for "inside an event" and didn't consider the
> possibility of arbitrarily long-lived event handlers.
> 
> > That just doesn't even apply to execCommand.
> 
> I agree. If the site doesn't make it clear that a particular site element is
> doing a copy to clipboard then no matter what kind of time-limit we impose
> the user experience would be that their clipboard was mysteriously stomped
> on by something, almost certainly discovered long after the event that did
> it.

OK, cool.  Thanks!

> It's still good to require it be in an event handler so some background page
> can't stomp your clipboard in a loop (see the eviltraps bug for a long list
> of pointless abusive things jerks do just because they can), but the
> time-limit isn't helping here.

Of course, that part I think everyone agrees on.  :-)

Comment 16

[(no longer active)]

Comment on attachment 8865533 [details] [diff] [review]
Disable the IsHandlingUserInput timeout for execCommand(copy/cut) commands

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

I think it's fullscreen that is special here, not this consumer.  In other words, you need to define a special API for fullscreen and switch that over, and change the semantics of IsHandlingUserInput() to only look at the event depth.  This function has many other callers in our code and I bet a lot of them are prone to similar failures, where random parts of the browser just break down on a slow machine under heavy load.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Disable the IsHandlingUserInput timeout for execCommand(copy/cut) commands

I apparently never posted the updated version of this patch for review and was wondering what was taking you so long :).

MozReview-Commit-ID: 341K1DEsVCg

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6dbb3f9f813
Disable the IsHandlingUserInput timeout for execCommand(copy/cut) commands, r=ehsan

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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