Consolidate browser and nsContentAreaDragDrop.cpp dropped link handlers

RESOLVED FIXED

Status

()

Core
Drag and Drop
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
Depends on: 545175, 546425
(Assignee)

Comment 1

7 years ago
Created attachment 427134 [details] [diff] [review]
consolidate both dropped link handlers and implement in a js component instead
Attachment #427134 - Flags: superreview?(jonas)
Attachment #427134 - Flags: review?(neil)
(Assignee)

Comment 2

7 years ago
Created attachment 427135 [details] [diff] [review]
browser changes
Attachment #427135 - Flags: review?(mano)
(Assignee)

Comment 3

7 years ago
Note that the patches have dependencies on patches in the dependant bugs.

Comment 4

7 years ago
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

7 years ago
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.]
(Assignee)

Comment 6

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
> >+        /^\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.
(Assignee)

Comment 8

7 years ago
Created attachment 427422 [details] [diff] [review]
address comments
Attachment #427134 - Attachment is obsolete: true
Attachment #427422 - Flags: superreview?(jonas)
Attachment #427422 - Flags: review?(neil)
Attachment #427134 - Flags: superreview?(jonas)
Attachment #427134 - Flags: review?(neil)
(Assignee)

Comment 9

7 years ago
Created attachment 427423 [details] [diff] [review]
browser parts, same as previous but with a couple of extra null checks
Attachment #427135 - Attachment is obsolete: true
Attachment #427423 - Flags: review?(mano)
Attachment #427135 - Flags: review?(mano)
(Assignee)

Updated

7 years ago
Blocks: 539476
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).
Attachment #427423 - Flags: review?(mano) → review-
(Assignee)

Comment 11

7 years ago
> > 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.
(Assignee)

Comment 12

7 years ago
Created attachment 429872 [details] [diff] [review]
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.
Attachment #427422 - Attachment is obsolete: true
Attachment #429872 - Flags: review?(neil)
Attachment #427422 - Flags: superreview?(jonas)
Attachment #427422 - Flags: review?(neil)
(Assignee)

Comment 13

7 years ago
Created attachment 429873 [details] [diff] [review]
update
Attachment #427423 - Attachment is obsolete: true
Attachment #429873 - Flags: review?(mano)
+    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...
(Assignee)

Comment 15

7 years ago
I wondered that too, but didn't remove it just in case. Do you think it could just be deleted?
Yeah.
Comment on attachment 429873 [details] [diff] [review]
update


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

Please use a lambda here.
Attachment #429873 - Flags: review?(mano)
(Assignee)

Comment 18

7 years ago
(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)
Yes.
(Assignee)

Comment 20

7 years ago
Created attachment 430157 [details] [diff] [review]
browser changes patch 4
[Checked in: Comment 40]
Attachment #429873 - Attachment is obsolete: true
Attachment #430157 - Flags: review?(mano)
Comment on attachment 430157 [details] [diff] [review]
browser changes patch 4
[Checked in: Comment 40]

r=mano
Attachment #430157 - Flags: review?(mano) → review+
(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.
(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 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.
(Assignee)

Comment 25

7 years ago
(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.
(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);
(Assignee)

Comment 27

7 years ago
(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.
(Assignee)

Comment 28

7 years ago
Created attachment 431676 [details] [diff] [review]
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
Attachment #429872 - Attachment is obsolete: true
Attachment #431676 - Flags: review?(neil)
Attachment #429872 - Flags: review?(neil)
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 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.
Attachment #431676 - Flags: review?(neil) → review-
(Assignee)

Comment 31

7 years ago
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?
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.
(Assignee)

Comment 33

7 years ago
(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?
(In reply to comment #33)
> When should javascript/data drops be allowed?

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

Comment 35

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 554400
(Assignee)

Comment 36

7 years ago
Created attachment 434292 [details] [diff] [review]
main changes, version 5
[Checked in: Comment 40]
Attachment #431676 - Attachment is obsolete: true
Attachment #434292 - Flags: review?(neil)
(Assignee)

Comment 37

7 years ago
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 on attachment 434292 [details] [diff] [review]
main changes, version 5
[Checked in: Comment 40]

>+  <script type="application/javascript" 
Nit: trailing space
Attachment #434292 - Flags: review?(neil) → review+
(Assignee)

Updated

7 years ago
Attachment #434292 - Flags: superreview?(jonas)
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 :)
Attachment #434292 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 40

7 years ago
http://hg.mozilla.org/mozilla-central/rev/049064ae127c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 41

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

Updated

7 years ago
Depends on: 560166
This caused bug 560166.
(Assignee)

Comment 43

7 years ago
The fix for bug 560166 re-enabled the test.

Updated

7 years ago
Depends on: 560658

Updated

7 years ago
Depends on: 560791

Updated

7 years ago
Blocks: 560889

Updated

7 years ago
Depends on: 561071

Updated

7 years ago
Depends on: 580710
Blocks: 497493

Updated

6 years ago
Blocks: 619355
Attachment #430157 - Attachment description: browser changes patch 4 → browser changes patch 4 [Checked in: Comment 40]
Attachment #434292 - Attachment description: main changes, version 5 → main changes, version 5 [Checked in: Comment 40]
No longer blocks: 560889
(Assignee)

Updated

6 years ago
Depends on: 616902
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(); :-(
Depends on: 656018
You need to log in before you can comment on or make changes to this bug.