Bug 1330979 - Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations, r=baku
authorMichael Layzell <michael@thelayzells.com>
Fri, 13 Jan 2017 14:35:51 -0500
changeset 374417 6e97457f78bd19c0935d9a105833a84760a4b408
parent 374416 d94f6ca9e7521922c75c34108efcc793685e8c96
child 374418 ef05499df1c39e5dfb39cbcadee1b6ead24ffb01
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1330979
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1330979 - Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations, r=baku 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
dom/events/DataTransferItem.cpp
--- a/dom/events/DataTransferItem.cpp
+++ b/dom/events/DataTransferItem.cpp
@@ -472,49 +472,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())) {
-    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
     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();
 }