Bug 1330979 - Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations. r=baku, a=lizzard
authorMichael Layzell <michael@thelayzells.com>
Fri, 13 Jan 2017 14:35:51 -0500
changeset 359389 3b73e8c0c9240d6fe875a1028902ae3a563e0404
parent 359388 e309c57e703df7e38cb3c0422c73da709d74ae21
child 359390 e1b44c560c28fd2605bbec4f5bfaa7592379a6c9
push id1324
push usermtabara@mozilla.com
push dateMon, 16 Jan 2017 13:07:44 +0000
treeherdermozilla-release@a01c49833940 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku, lizzard
Bug 1330979 - Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations. r=baku, a=lizzard In our previous code, we would accidentially not perform the correct security checks when retreiving data from DataTransferItems in some situations. The regressing patch fixed this behavior and ensured that we always perform the correct security checks, making any "insecure" accesses (such as accesses to file data from the OS in DragEnter) would raise an NS_ERROR_DOM_SECURTY_ERROR. This behavior is not consistent with Chrome, and thus OneDrive would accidentally trigger an exception and break its d&d handling logic. With this patch our behavior for "insecure" accesses to file data from the OS is more in line with Chrome's, in that we now don't raise an exception, but instead just produce the value "null" when the data should not be avaliable yet. From my quick test this fixes the problem with OneDrive, and should be a fairly trivial patch to uplift to Beta, with very little risk. This patch doesn't include a test, as Drag and Drop is a very difficult component to test in automation, however I am currently working on defining a set of manual tests for the Drag and Drop component, and I'll make sure that our behavior for "insecure" accesses is tested in those tests in the future. MozReview-Commit-ID: 4pnPyL1tgcV
--- a/dom/events/DataTransferItem.cpp
+++ b/dom/events/DataTransferItem.cpp
@@ -476,49 +476,50 @@ DataTransferItem::Data(nsIPrincipal* aPr
      mDataTransfer->GetEventMessage() != ePaste);
   // Check if the caller is allowed to access the drag data. Callers with
   // chrome privileges can always read the data. During the
   // drop event, allow retrieving the data except in the case where the
   // source of the drag is in a child frame of the caller. In that case,
   // we only allow access to data of the same principal. During other events,
   // only allow access to the data with the same principal.
+  //
+  // We don't want to fail with an exception in this siutation, rather we want
+  // to just pretend as though the stored data is "nullptr". This is consistent
+  // with Chrome's behavior and is less surprising for web applications which
+  // don't expect execptions to be raised when performing certain operations.
   if (Principal() && checkItemPrincipal &&
       !aPrincipal->Subsumes(Principal())) {
     return nullptr;
   if (!variant) {
     return nullptr;
   nsCOMPtr<nsISupports> data;
   nsresult rv = variant->GetAsISupports(getter_AddRefs(data));
   if (NS_SUCCEEDED(rv) && data) {
     nsCOMPtr<EventTarget> pt = do_QueryInterface(data);
     if (pt) {
       nsIScriptContext* c = pt->GetContextForEventHandlers(&rv);
       if (NS_WARN_IF(NS_FAILED(rv) || !c)) {
-        aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
         return nullptr;
       nsIGlobalObject* go = c->GetGlobalObject();
       if (NS_WARN_IF(!go)) {
-        aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
         return nullptr;
       nsCOMPtr<nsIScriptObjectPrincipal> sp = do_QueryInterface(go);
       MOZ_ASSERT(sp, "This cannot fail on the main thread.");
       nsIPrincipal* dataPrincipal = sp->GetPrincipal();
       if (NS_WARN_IF(!dataPrincipal || !aPrincipal->Equals(dataPrincipal))) {
-        aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
         return nullptr;
   return variant.forget();