846635 - Use asynchronous getCharsetForURI in getShortcutOrURI

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2274

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Raymond Lee [:raymondlee]]

Mano: do you have a chance to review this patch? thanks

Comment 3

[Asaf Romano (gone)]

Comment on attachment 720263 [details] [diff] [review]
v1

>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

> function getShortcutOrURI(aURL, aPostDataRef, aMayInheritPrincipal) {

So, first of all, the signature must change, and because JS doesn't enforce a signature, the function must be renamed.

The new method shouldn't take out params, but rather return any value as part of in the promise resultion value.

>+  return Task.spawn(function() {
>+    // Initialize outparam to false
>+    if (aMayInheritPrincipal)
>+      aMayInheritPrincipal.value = false;
>+
>+    var shortcutURL = null;
>+    var keyword = aURL;
>+    var param = "";

nit: s/var/let for new code (here and elsewhere).

>+    var offset = aURL.indexOf(" ");
>+    if (offset > 0) {
>+      keyword = aURL.substr(0, offset);
>+      param = aURL.substr(offset + 1);
>+    }
>+
>+    if (!aPostDataRef)
>+      aPostDataRef = {};
>+
>+    var engine = Services.search.getEngineByAlias(keyword);
>+    if (engine) {
>+      var submission = engine.getSubmission(param);
>+      aPostDataRef.value = submission.postData;
>+      throw new Task.Result(submission.uri.spec);

So I expect the "return value" to be some kind of object now.

>@@ -3241,42 +3246,46 @@ var newTabButtonObserver = {
> 
>   onDragExit: function (aEvent)
>   {
>   },
> 
>   onDrop: function (aEvent)
>   {
>     let url = browserDragAndDrop.drop(aEvent, { });
>-    var postData = {};
>-    url = getShortcutOrURI(url, postData);
>-    if (url) {
>-      // allow third-party services to fixup this URL
>-      openNewTabWith(url, null, postData.value, aEvent, true);
>-    }
>+    Task.spawn(function() {
  
Next fundamental issue: few callers can now wait for getShortcutOrURI to finish at the same time, and, if the current tab is the target for the new urls, each getShortcutOrURI call will resolve at some point, forcing open* to be called multiple times with no guaranteed order.

This isn't acceptable. We should ensure that (1) It's the last *user-action* that takes precedence as to which url is loaded (2) open* is called just once.

I haven't reviewed the rest of the patch as addressing this issue may change the code much.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v2 diff -w patch

(In reply to Mano from comment #3)
> Comment on attachment 720263 [details] [diff] [review]
> v1
> 
> >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
> 
> > function getShortcutOrURI(aURL, aPostDataRef, aMayInheritPrincipal) {
> 
> So, first of all, the signature must change, and because JS doesn't enforce
> a signature, the function must be renamed.
> 
> The new method shouldn't take out params, but rather return any value as
> part of in the promise resultion value.

Changed to getShortcutOrURIAndPostData(url)

> 
> >+  return Task.spawn(function() {
> >+    // Initialize outparam to false
> >+    if (aMayInheritPrincipal)
> >+      aMayInheritPrincipal.value = false;
> >+
> >+    var shortcutURL = null;
> >+    var keyword = aURL;
> >+    var param = "";
> 
> nit: s/var/let for new code (here and elsewhere).

Done

> 
> >+    var offset = aURL.indexOf(" ");
> >+    if (offset > 0) {
> >+      keyword = aURL.substr(0, offset);
> >+      param = aURL.substr(offset + 1);
> >+    }
> >+
> >+    if (!aPostDataRef)
> >+      aPostDataRef = {};
> >+
> >+    var engine = Services.search.getEngineByAlias(keyword);
> >+    if (engine) {
> >+      var submission = engine.getSubmission(param);
> >+      aPostDataRef.value = submission.postData;
> >+      throw new Task.Result(submission.uri.spec);
> 
> So I expect the "return value" to be some kind of object now.

Done.

> 
> >@@ -3241,42 +3246,46 @@ var newTabButtonObserver = {
> > 
> >   onDragExit: function (aEvent)
> >   {
> >   },
> > 
> >   onDrop: function (aEvent)
> >   {
> >     let url = browserDragAndDrop.drop(aEvent, { });
> >-    var postData = {};
> >-    url = getShortcutOrURI(url, postData);
> >-    if (url) {
> >-      // allow third-party services to fixup this URL
> >-      openNewTabWith(url, null, postData.value, aEvent, true);
> >-    }
> >+    Task.spawn(function() {
>   
> Next fundamental issue: few callers can now wait for getShortcutOrURI to
> finish at the same time, and, if the current tab is the target for the new
> urls, each getShortcutOrURI call will resolve at some point, forcing open*
> to be called multiple times with no guaranteed order.
> 
> This isn't acceptable. We should ensure that (1) It's the last *user-action*
> that takes precedence as to which url is loaded (2) open* is called just
> once.

I've added lastURLForCurrentTab object class for that purpose, and applied to code which need to wait for getShortcutOrURIAndPostData() and open url in current tab.

> 
> I haven't reviewed the rest of the patch as addressing this issue may change
> the code much.

Comment 5

[Asaf Romano (gone)]

I'm afraid the lastURLForCurrentTab.update thing isn't enough. We sure have some callers that change the currently-loaded page without going through getShortcutURI. They will most certainly break in the following scenario:
1) url is pasted etc.
2) some ui command that doesn't go through getShortcutURI loads  page.
3) tge promise is resolved, a new page is loaded overriding the last ui action.

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to Mano from comment #5)
> I'm afraid the lastURLForCurrentTab.update thing isn't enough. We sure have
> some callers that change the currently-loaded page without going through
> getShortcutURI. They will most certainly break in the following scenario:
> 1) url is pasted etc.
> 2) some ui command that doesn't go through getShortcutURI loads  page.
> 3) tge promise is resolved, a new page is loaded overriding the last ui
> action.

That's tricky. Is there a central place e.g. method which every caller would invoke?

Comment 7

[Asaf Romano (gone)]

It's tricky, but it's manageable. For starters, most callers use the helpers provided by utilityOverlay.js (openURLIn etc). Once they're taken care of, we'll need to find any callers that bypass the utilityOverlay methods, and fix them. There shouldn't be *too* many.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v3 -w diff

(In reply to Mano from comment #7)
> It's tricky, but it's manageable. For starters, most callers use the helpers
> provided by utilityOverlay.js (openURLIn etc). Once they're taken care of,
> we'll need to find any callers that bypass the utilityOverlay methods, and
> fix them. There shouldn't be *too* many.

I have done some digging and those helper methods in utilityOverlay.js would eventually invoke loadURIWithFlags in tabbrowser.xml.  Therefore, I've added lastURLForCurrentTab.update() to few places in tabbrowser.xml.

Comment 9

[Raymond Lee [:raymondlee]]

Comment on attachment 738961 [details] [diff] [review]
v3 -w diff

@mano: please let me know what this patch looks ok.  Thanks!

Comment 10

[Asaf Romano (gone)]

I'll get to this patch in the next couple of days. Sorry this is taking so long.

Comment 11

[Asaf Romano (gone)]

Reviewing this now finally. Sorry again. Raymond, any chance you could also attach the full diff (without -w, that is)?

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v3 without -w

Here is the without -w version

Comment 13

[Asaf Romano (gone)]

Couple of thoughts meanwhile:
1) minor: If (and I'm pretty sure that's what we're going to end up doing) this is indeed going to be managed in tabbrowser.xml, your managing object should live there (as a filed).
2) major: So, what about background tabs? I cannot recall if we allow dropping keywords on background tabs "yet", but it's a valid use case anyway. It seems to me that we should manage the loads for each tab (meaning that the managing object should probably be set on the <browser> element).

What do you think?

Comment 14

[Asaf Romano (gone)]

By the way, if you're ever on IRC (moznet), please message me.

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to Mano from comment #13)
> Couple of thoughts meanwhile:
> 1) minor: If (and I'm pretty sure that's what we're going to end up doing)
> this is indeed going to be managed in tabbrowser.xml, your managing object
> should live there (as a filed).
> 2) major: So, what about background tabs? I cannot recall if we allow
> dropping keywords on background tabs "yet", but it's a valid use case
> anyway. It seems to me that we should manage the loads for each tab (meaning
> that the managing object should probably be set on the <browser> element).
> 
> What do you think?

Yes, it makes sense to manage the loads for each tab.  Actually, I have tested on Mac and we can actually drop keyword / link on a background tab but it doesn't always work :P

Here is the flow:
1) When user drops a link to a tab, we first determine which <browser> the url is going to load into.
2) Update the |lastURL| property which attaches to the <browser> element
3) After executing async |getCharsetForURI|, check the |lastURL| on the <browser> element.  If the url and |lastURL|, loads the url and clear the |lastURL|.

All the code which sets |lastURL| should be added to tabbrowser.xml, right?

Does it make sense?

(In reply to Mano from comment #14)
> By the way, if you're ever on IRC (moznet), please message me.

What's your username? I am raymondlee, #firefox.

Comment 16

[Asaf Romano (gone)]

Why did you remove the flags?

(Mano on IRC)

Comment 17

[Asaf Romano (gone)]

For what it's worth, I do think that making this independent of the current-tab is a requirement, so it won't unexpectedly block future work on browser features.

Comment 18

[Asaf Romano (gone)]

> All the code which sets |lastURL| should be added to tabbrowser.xml, right?

Yes.

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to Mano from comment #16)
> Why did you remove the flags?
> 
> (Mano on IRC)

I have updated the patch to set the lastURI flag on browser.

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: v4 -w

Comment 21

[Asaf Romano (gone)]

We're getting close!

So, i have to say the last-url concept is somewhat counter-intuitive, or over-complex. I think it could be simplified a bit.

What each caller actually needs to know is whether there was any user-initiated url change while the promise was pending: in other words, whether onLocationChange was called for the top window of the browser in which the url should be loaded (and, btw, this means that the lastURL solution doesn't cover all cases... what about clicking links? I guess we should update onLocationChange. There's a listener for that in tabbrowser.xml).

So, I think the simplest solution (which should be easy to implement over the current patch) is to have on each browser a lastLocationChange property, which should be set to Date.now() in onLocationChange (for top level loads). Then, in each caller, you cache it and then check if it changed in the promise's callback. The callers then don't need to call updateLastURL and so on. They just compare times.

Let me apologize again for the incremental review. It's just that this is the kind of functionality that doesn't play nice with asynchronous APIs, and we have almost no similar cases (yet..) to base the solution upon.

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: v5

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: v5 -w

Comment 24

[Asaf Romano (gone)]

Comment on attachment 762474 [details] [diff] [review]
v5

OK, let's try this. I'm going to file a follow up on a more generic solution to this kind of situations ("canceling" an async opetation).

Please land this on Mozilla-Try first.

Comment 25

[Asaf Romano (gone)]

Please also file a follow up on decent tests for this code.

Comment 26

[Raymond Lee [:raymondlee]]

(In reply to Mano from comment #25)
> Please also file a follow up on decent tests for this code.

Filed bug 886639

Comment 27

[Raymond Lee [:raymondlee]]

(In reply to Mano from comment #24)
> Comment on attachment 762474 [details] [diff] [review]
> v5
> 
> Please land this on Mozilla-Try first.

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=cf4e4faf153f

Comment 28

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #27)
> (In reply to Mano from comment #24)
> > Comment on attachment 762474 [details] [diff] [review]
> > v5
> > 
> > Please land this on Mozilla-Try first.
> 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=cf4e4faf153f

Th results look ok, shall we land this on inbound?

Comment 29

[Asaf Romano (gone)]

Sure go ahead.

Comment 30

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19845a51b863

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19845a51b863

Comment 33

[Asaf Romano (gone)]

This is not a documented API, or precisely speaking an API at all.

Comment 34

[Jorge Villalobos [:jorgev] (he/him)]

This breaks a few add-ons. See bug 919484 for an example.