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 353597 e3c132ec8b1f1843f24fb207e46fdaf626c37aae
parent 353596 5538b0241b7391ca2f73f64147316540f73502df
child 353598 02d0ba8567f3de09c9df0a95138d0c4e7b5a57a0
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku, lizzard
bugs1330979
milestone52.0a2
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
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();
 }