545714 - Consolidate browser and nsContentAreaDragDrop.cpp dropped link handlers

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Neil Deakin]

Currently both browser.js and nsContentAreaDragDrop.cpp implement handlers for dropped links, yet only one should exist. Firefox uses the former, and due to the way the browser binding is initializes uses only parts of the nsContentAreaDragDrop.cpp handling. Embedding uses the former exclusively. Both should be consolidated into a single implementation.

In addition, the listeners should be uses the system event group.

The patch I am working on uses a component for this. It would be nice not to use one though, but the need to work in an embedded case limits this a bit.

Comment 1

[Neil Deakin]

Attachment: consolidate both dropped link handlers and implement in a js component instead

Comment 2

[Neil Deakin]

Attachment: browser changes

Comment 3

[Neil Deakin]

Note that the patches have dependencies on patches in the dependant bugs.

Comment 4

[[e-mail]]

Comment on attachment 427134 [details] [diff] [review]
consolidate both dropped link handlers and implement in a js component instead

>+          var split = dt.getData(type).split("\n");
>+          return [split[0], split[1]];
Why not just return the split result?

>+      var name = file instanceof Ci.nsIFile ? file.leafName : "";
>+
>+      var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                                .getService(Ci.nsIIOService);
>+      var fileHandler = ioService.getProtocolHandler("file")
>+                                 .QueryInterface(Ci.nsIFileProtocolHandler);
>+      return [fileHandler.getURLSpecFromFile(file), name];
How are you expecting to get the url spec from the file for non-files?

>+    return [ , ];
[,].length is 1, did you mean that? ([] would probably work just as well.)

>+    // sure the source document can load the dropped URI. We don't
>+    // so much care about creating the real URI here
Except checkLoadURI has to do this anyway, so why not save it the trouble?

>+    uriString = uriString.replace(/^\s*|\s*$/g, '');
What, again? Didn't you already do that? Oh, not in the split case.

>+    secMan.checkLoadURIStr(sourceDocument.documentURI, uriString,
>+                           secMan.STANDARD);
You should be calling checkLoadURIWithPrincipal. See contentAreaUtils.js

>+      var sourceRoot =
>+        sourceDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                  .getInterface(Ci.nsIWebNavigation)
>+                                  .QueryInterface(Ci.nsIDocShellTreeItem)
>+                                  .sameTypeRootTreeItem;
Could use sourceDocument.defaultView.top

>+  dropLink: function(aEvent, aName)
>+  {
>+    if (aName)
XPConnect will always set aName for an outparam.

>+        /^\s*(javascript|data):/.test(url))
Use the DISALLOW_SCRIPT_OR_DATA flag perhaps?

[To be continued...]

Comment 5

[[e-mail]]

Comment on attachment 427134 [details] [diff] [review]
consolidate both dropped link handlers and implement in a js component instead

>+class nsContentAreaDragDrop
What does this class do? You seem to have removed all of the consumers.

>+  piTarget->GetSystemEventGroup(getter_AddRefs(sysGroup));
Why the system event group?

>+                                      NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
Were we allowing untrusted events before? I thought we weren't.

>-      <field name="mDragDropHandler">
>-        null
>-      </field>
...
>+      <field name="droppedLinkHandler">
>+        null
>+      </field>
Nit: could have just renamed the existing field.

>+            this.loadURI(uri, null, "");
[I wonder why we don't set the referrer for a drop.]

Comment 6

[Neil Deakin]

(In reply to comment #5)
> (From update of attachment 427134 [details] [diff] [review])
> >+class nsContentAreaDragDrop
> What does this class do? You seem to have removed all of the consumers.
> 

It currently only holds a single static method GetDragData called from nsEventStateManager::DetermineDragTarget. The method determines what data should be dragged from the selection or focus.

It could probably just be converted to just the object it indirectly creates and calls to do the real work.


> >+  piTarget->GetSystemEventGroup(getter_AddRefs(sysGroup));
> Why the system event group?

Bug 539476 will change the editor listeners to be in the system group so that they act more like default handlers. Thus, if the content listener isn't in the system group is runs before the editor ones when it should run after during event propagation. The easiest fix was to move to the system group.
 
> >-      <field name="mDragDropHandler">
> >-        null
> >-      </field>
> ...
> >+      <field name="droppedLinkHandler">
> >+        null
> >+      </field>
> Nit: could have just renamed the existing field.

The former is internal to browser.xml while the latter is expected to be set from outside.

Comment 7

[Neil Deakin]

> >+        /^\s*(javascript|data):/.test(url))
> Use the DISALLOW_SCRIPT_OR_DATA flag perhaps?

I just copied this code from browser.js but I suspect because it would prevent data or javascript urls from being dropped on the url field or bookmarks bar.

Comment 8

[Neil Deakin]

Attachment: address comments

Comment 9

[Neil Deakin]

Attachment: browser parts, same as previous but with a couple of extra null checks

Comment 10

[Asaf Romano (gone)]

Comment on attachment 427423 [details] [diff] [review]
browser parts, same as previous but with a couple of extra null checks

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1144,16 +1144,19 @@ function prepareForStartup() {
>       var arrayArgComponents = window.arguments[1].split("=");
>       if (arrayArgComponents) {
>         //we should "inherit" the charset menu setting in a new window
>         getMarkupDocumentViewer().defaultCharacterSet = arrayArgComponents[1];
>       }
>     }
>   }
> 
>+  // add the callback for handling links dragged onto the browser
>+  gBrowser.droppedLinkHandler = handleDroppedLink;
>+

nit: UFirst the comment and it with period.

> function getShortcutOrURI(aURL, aPostDataRef) {
>+  if (!aURL)
>+    return "";
>+

Er, that param is not optional.  Fix the callers instead.

>+    if (!this.linkHandlerService) {
>+      this.linkHandlerService = Components.classes["@mozilla.org/content/dropped-link-handler;1"].
>+                                  getService(Components.interfaces.nsIDroppedLinkHandler);
>+    }
>+

Cc and Ci should be used in browser.js.  Also, please consider setting it as a global smart getter (search for "elementGlobal" in browser.js).

> 
> var proxyIconDNDObserver = {
>   onDragStart: function (aEvent, aXferData, aDragAction)
>     {
>       if (gProxyFavIcon.getAttribute("pageproxystate") != "valid")
>         return;
> 
>       var value = content.location.href;
>@@ -2770,18 +2753,17 @@ var proxyIconDNDObserver = {
>       dt.setData("text/plain", value);
>       dt.setData("text/html", htmlString);
>     }
> }
> 
> var homeButtonObserver = {
>   onDrop: function (aEvent)
>     {
>-      let url = browserDragAndDrop.getDragURLFromDataTransfer(aEvent.dataTransfer)[0];
>-      setTimeout(openHomeDialog, 0, url);
>+      setTimeout(openHomeDialog, 0, browserDragAndDrop.drop(aEvent, { }));

Never pass a an object's method to setTimeout.  It results in unexpected results later when someone tries to use |this|.

> 
> var bookmarksButtonObserver = {
>   onDrop: function (aEvent)
>   {
>-    let [url, name] = browserDragAndDrop.getDragURLFromDataTransfer(aEvent.dataTransfer);
>+    var name = { };

Why var?

>+    let url = browserDragAndDrop.drop(aEvent, name)

Missing semicolon.


>+function handleDroppedLink(event, url, name)
>+{
>+ ......

Prefer "let" in new code.

>+  // keep the event from being handled by the dragDrop listeners
>+  // built-in to gecko if they happen to be above us.

UFirst.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>+      <field name="droppedLinkHandler">
>+        null
>+      </field>
>+

This isn't necessary (and if you're going to use a global var, the field won't be needed at all).

Comment 11

[Neil Deakin]

> > var homeButtonObserver = {
> >   onDrop: function (aEvent)
> >     {
> >-      let url = browserDragAndDrop.getDragURLFromDataTransfer(aEvent.dataTransfer)[0];
> >-      setTimeout(openHomeDialog, 0, url);
> >+      setTimeout(openHomeDialog, 0, browserDragAndDrop.drop(aEvent, { }));
> 
> Never pass a an object's method to setTimeout.  It results in unexpected
> results later when someone tries to use |this|.

I think you misread this as passing browserDragAndDrop.drop to setTimeout. It actually calls the method and passes the result which is a url.

Comment 12

[Neil Deakin]

Attachment: address comments

- change to use let
- use Services.jsm to store the droppedLink service
- found a bug where dragging a bookmark to the home button didn't work. This is because the same document check should only apply to some drops. Added an argument to canDropLink to handle this difference.

Comment 13

[Neil Deakin]

Attachment: update

Comment 14

[Asaf Romano (gone)]

+    case "navigator:view-source":
+      viewSource(url);
+      break;

When could that happen? I know that it's copied over, but it looks and smells like trash...

Comment 15

[Neil Deakin]

I wondered that too, but didn't remove it just in case. Do you think it could just be deleted?

Comment 16

[Asaf Romano (gone)]

Yeah.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 429873 [details] [diff] [review]
update


>+  drop : function (aEvent, aName)
>+  {
>+    return Services.droppedLinkHandler.dropLink(aEvent, aName);
>+  }
>+}

Please use a lambda here.

Comment 18

[Neil Deakin]

(In reply to comment #17)
> >+  drop : function (aEvent, aName)
> >+  {
> >+    return Services.droppedLinkHandler.dropLink(aEvent, aName);
> >+  }
> >+}
> 
> Please use a lambda here.

Do you mean:

 drop: function (aEvent, aName) Services.droppedLinkHandler.dropLink(aEvent, aName)

Comment 19

[Asaf Romano (gone)]

Yes.

Comment 20

[Neil Deakin]

Attachment: browser changes patch 4 [Checked in: Comment 40]

Comment 21

[Asaf Romano (gone)]

Comment on attachment 430157 [details] [diff] [review]
browser changes patch 4
[Checked in: Comment 40]

r=mano

Comment 22

[[e-mail]]

(In reply to comment #7)
> > >+        /^\s*(javascript|data):/.test(url))
> > Use the DISALLOW_SCRIPT_OR_DATA flag perhaps?
> I just copied this code from browser.js but I suspect because it would prevent
> data or javascript urls from being dropped on the url field or bookmarks bar.
This was quoted from ContentAreaDropListener.dropLink so I don't see how.

Comment 23

[[e-mail]]

(In reply to comment #14)
> +    case "navigator:view-source":
> +      viewSource(url);
> +      break;
> When could that happen? I know that it's copied over, but it looks and smells
> like trash...
Back in the Mozilla Suite days, the view source window used to use the same contentAreaDD observer as the browser window. At some point someone split up the view source window from the browser, however the call still exists and ironically the event handler was updated to the new event name...

Comment 24

[[e-mail]]

Comment on attachment 429872 [details] [diff] [review]
address comments

>+  gBrowser.droppedLinkHandler = function (event, url, name) {
>+    viewSource(url)
>+    event.preventDefault();
Which this in fact will fix, right?

>-  <vbox id="appcontent" flex="1" ondrop="contentAreaDNDObserver.drop(event);">
>+  <vbox id="appcontent" flex="1">
There seems to be one in viewPartialSource.xul too.

Comment 25

[Neil Deakin]

(In reply to comment #22)
> > > >+        /^\s*(javascript|data):/.test(url))
> > > Use the DISALLOW_SCRIPT_OR_DATA flag perhaps?
...
> This was quoted from ContentAreaDropListener.dropLink so I don't see how.

Bug 547813 deals with adding this flag.

Comment 26

[[e-mail]]

(In reply to comment #25)
>>>+        /^\s*(javascript|data):/.test(url))
>>Use the DISALLOW_SCRIPT_OR_DATA flag perhaps?
>Bug 547813 deals with adding this flag.
And will the existing string-based check be removed at the same time?

(From update of attachment 429872 [details] [diff] [review])
>+        let linkHandler = Components.classes["@mozilla.org/content/dropped-link-handler;1"].
>+                            getService(Components.interfaces.nsIDroppedLinkHandler);
This is correct style when using Cc but for Components.classes I'd prefer
Components.classes["@mozilla.org/content/dropped-link-handler;1"]
          .getService(Components.interfaces.nsIDroppedLinkHandler);

Comment 27

[Neil Deakin]

(In reply to comment #26)
> (In reply to comment #25)
> >>>+        /^\s*(javascript|data):/.test(url))
> >>Use the DISALLOW_SCRIPT_OR_DATA flag perhaps?
> >Bug 547813 deals with adding this flag.
> And will the existing string-based check be removed at the same time?

I've made a note to that effect in the other bug.

Comment 28

[Neil Deakin]

Attachment: main changes, version 4

This patch addresses the following:

- fixes an issue with the urlbar that prevented text from being dropped
- adds changes to viewPartialSource
- fixes an accessibility test
- updates for bug 549349
- changed to use Cc

Comment 29

[[e-mail]]

Comment on attachment 431676 [details] [diff] [review]
main changes, version 4

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
This belongs in Mano's patch, no?

>+    secMan.checkLoadURIStr(sourceURI, uriString, secMan.STANDARD);
Wait, we're back to checkLoadURIStr?

>+    // valid urls don't contain spaces ' '; if we have a space it
>+    // isn't a valid url, or if it's a javascript: or data: url,
>+    // bail out
>+    if (!url || !url.length || url.indexOf(" ", 0) != -1 ||
>+        /^\s*(javascript|data):/.test(url))
>+      return "";
>+
>+    try {
>+      this._securityCheck(dataTransfer, url);
>+    } catch (ex) {
>+      if (ex.result == Components.results.NS_ERROR_MALFORMED_URI)
>+        return "";
Maybe we would be better off simply creating a URI object in the first place?

> EXTRA_COMPONENTS = \
> 		$(srcdir)/nsBadCertHandler.js \
> 		contentSecurityPolicy.js \
>+		ContentAreaDropListener.js \
What's the precedent for the leading capital letter?

Comment 30

[[e-mail]]

Comment on attachment 431676 [details] [diff] [review]
main changes, version 4

>+    let sourceNode = dataTransfer.mozSourceNode;
>+    if (!sourceNode)
>+      return uriString;
>+
>+    let sourceDocument = sourceNode.ownerDocument;
>+    let sourceURI = sourceDocument ? sourceDocument.documentURI : "file:///";
>+
>+    // uriString is a valid URI, so do the security check.
>+    let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].
>+                   getService(Ci.nsIScriptSecurityManager);
>+    secMan.checkLoadURIStr(sourceURI, uriString, secMan.STANDARD);
This is all wrong. If you have a source node then you should use its principal as before. If you don't have a source node then you should ask the secMan for a file:/// principal. Then you can restore the checkLoadURIWithPrincipal call.

Comment 31

[Neil Deakin]

OK, how about the following:

  _validateURI: function(dataTransfer, uriString)
  {
    if (!uriString)
      return "";

    // Strip leading and trailing whitespace, then try to create a
    // URI from the dropped string. If that succeeds, we're
    // dropping a URI and we need to do a security check to make
    // sure the source document can load the dropped URI.
    uriString = uriString.replace(/^\s*|\s*$/g, '');

    // valid urls don't contain spaces ' '; if we have a space it
    // isn't a valid url, or if it's a javascript: or data: url,
    // bail out
    if (!uriString.length || uriString.indexOf(" ", 0) != -1 ||
        /^\s*(javascript|data):/.test(uriString))
      return "";

    let uri;
    try {
      // Check that the uri is valid first and return an empty string if not.
      // It may just be plain text and should be ignored here
      uri = Cc["@mozilla.org/network/io-service;1"].
              getService(Components.interfaces.nsIIOService).
              newURI(uriString, null, null);
    } catch (ex) { }
    if (!uri)
      return "";

    // uriString is a valid URI, so do the security check.
    let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].
                   getService(Ci.nsIScriptSecurityManager);
    let sourceNode = dataTransfer.mozSourceNode;
    // Use file:/// as the default uri so that drops of file URIs are always allowed
    if (sourceNode)
      secMan.checkLoadURIStrWithPrincipal(sourceNode.nodePrincipal, uriString, secMan.STANDARD);
    else
      secMan.checkLoadURIStr("file:///", uriString, secMan.STANDARD);

    return uriString;
  },

Note that I changed the method name and moved all url checking within it.

Would it be better to create an nsIURI for file:/// and call checkLoadURIStrWithPrincipal with it?

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Is that function meant to be used as a replacement for dragDropSecurityCheck? If so, I don't think it should be bailing out early when uriString is a javascript:/data: URI or contains whitespace. Seems like that would make it easy to work around the fix for bug 546909.

Otherwise that looks OK to me. It probably would be best to use checkLoadURIStrWithPrincipal(secman.getCodeBasePrincipal(makeURI("file:///")), uriString, 0), I guess, given bug 327244.

Comment 33

[Neil Deakin]

(In reply to comment #32)
> Is that function meant to be used as a replacement for dragDropSecurityCheck?
> If so, I don't think it should be bailing out early when uriString is a
> javascript:/data: URI or contains whitespace. Seems like that would make it
> easy to work around the fix for bug 546909.

A null string is returned in this case and a drop doesn't occur. If anything, it should probably allow a drop in some situations.

When should javascript/data drops be allowed?

Comment 34

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #33)
> When should javascript/data drops be allowed?

Right now they're always allowed. Bug 547813 was about changing that.

Comment 35

[Neil Deakin]

OK, I'll just remove these lines:

    if (!uriString.length || uriString.indexOf(" ", 0) != -1 ||
        /^\s*(javascript|data):/.test(uriString))
      return "";

Only the content area drop handler does this currently. I'll leave it for that case only.

Comment 36

[Neil Deakin]

Attachment: main changes, version 5 [Checked in: Comment 40]

Comment 37

[Neil Deakin]

I found a bug in this when dragging plain text.

In validateURI, we should return uriString if not a valid uri. This way we handle the plain text as is. Perhaps the link handler shouldn't do this, but this is the way the existing browser drop code works in each situation, and also allows bookmark keywords to work properly.

    try {
      // Check that the uri is valid first and return an empty string if not.
      // It may just be plain text and should be ignored here
      uri = Cc["@mozilla.org/network/io-service;1"].
              getService(Components.interfaces.nsIIOService).
              newURI(uriString, null, null);
    } catch (ex) { }
    if (!uri)
-      return "";
+      return uriString;

I'll fix this up.

Comment 38

[[e-mail]]

Comment on attachment 434292 [details] [diff] [review]
main changes, version 5
[Checked in: Comment 40]

>+  <script type="application/javascript" 
Nit: trailing space

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 434292 [details] [diff] [review]
main changes, version 5
[Checked in: Comment 40]

Let me know if there was anything in particular you wanted me to check. Only looked at it from a high level, though it looked great from there :)

Comment 40

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/049064ae127c

Comment 41

[Neil Deakin]

There's something wrong with the test so I disabled it temporarily

Comment 42

[Boris Zbarsky [:bzbarsky]]

This caused bug 560166.

Comment 43

[Neil Deakin]

The fix for bug 560166 re-enabled the test.

Comment 44

[[e-mail]]

Comment on attachment 434292 [details] [diff] [review]
main changes, version 5
[Checked in: Comment 40]

>+    uriString = uriString.replace(/^\s*|\s*$/g, '');
Bah, you did this three times and I still didn't notice that you could have just as easily used .trim(); :-(