Last Comment Bug 846635 - Use asynchronous getCharsetForURI in getShortcutOrURI
: Use asynchronous getCharsetForURI in getShortcutOrURI
Status: RESOLVED FIXED
: addon-compat
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla25
Assigned To: Raymond Lee [:raymondlee]
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 834543 1000458 1044351
Blocks: asyncHistory 886639 880922
  Show dependency treegraph
 
Reported: 2013-02-28 22:19 PST by Raymond Lee [:raymondlee]
Modified: 2014-12-26 21:47 PST (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (30.31 KB, patch)
2013-03-02 03:01 PST, Raymond Lee [:raymondlee]
asaf: review-
Details | Diff | Splinter Review
v2 diff -w patch (19.50 KB, patch)
2013-04-12 07:48 PDT, Raymond Lee [:raymondlee]
asaf: review-
Details | Diff | Splinter Review
v3 -w diff (21.90 KB, patch)
2013-04-18 03:22 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v3 without -w (33.64 KB, patch)
2013-06-03 09:26 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v4 (34.44 KB, patch)
2013-06-13 01:11 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v4 -w (22.70 KB, patch)
2013-06-13 01:11 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v5 (33.03 KB, patch)
2013-06-13 20:35 PDT, Raymond Lee [:raymondlee]
asaf: review+
Details | Diff | Splinter Review
v5 -w (21.76 KB, patch)
2013-06-13 20:36 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
Patch for checkin (33.32 KB, patch)
2013-06-27 08:43 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Comment 1 Raymond Lee [:raymondlee] 2013-03-02 03:01:30 PST
Created attachment 720263 [details] [diff] [review]
v1
Comment 2 Raymond Lee [:raymondlee] 2013-03-26 01:18:50 PDT
Mano: do you have a chance to review this patch? thanks
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-04-08 04:28:56 PDT
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] 2013-04-12 07:48:14 PDT
Created attachment 736816 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-04-16 09:08:25 PDT
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] 2013-04-16 09:36:04 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-04-16 09:51:16 PDT
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] 2013-04-18 03:22:39 PDT
Created attachment 738961 [details] [diff] [review]
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] 2013-05-19 22:25:34 PDT
Comment on attachment 738961 [details] [diff] [review]
v3 -w diff

@mano: please let me know what this patch looks ok.  Thanks!
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-05-20 01:56:06 PDT
I'll get to this patch in the next couple of days. Sorry this is taking so long.
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-03 09:22:02 PDT
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] 2013-06-03 09:26:05 PDT
Created attachment 757449 [details] [diff] [review]
v3 without -w

Here is the without -w version
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-03 09:27:15 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-03 09:47:05 PDT
By the way, if you're ever on IRC (moznet), please message me.
Comment 15 Raymond Lee [:raymondlee] 2013-06-06 21:14:17 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-13 00:35:09 PDT
Why did you remove the flags?

(Mano on IRC)
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-13 00:36:39 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-13 00:37:26 PDT
> All the code which sets |lastURL| should be added to tabbrowser.xml, right?

Yes.
Comment 19 Raymond Lee [:raymondlee] 2013-06-13 01:11:12 PDT
Created attachment 761906 [details] [diff] [review]
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] 2013-06-13 01:11:32 PDT
Created attachment 761908 [details] [diff] [review]
v4 -w
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-13 09:40:58 PDT
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] 2013-06-13 20:35:45 PDT
Created attachment 762474 [details] [diff] [review]
v5
Comment 23 Raymond Lee [:raymondlee] 2013-06-13 20:36:18 PDT
Created attachment 762476 [details] [diff] [review]
v5 -w
Comment 24 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-24 10:20:08 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-24 10:21:55 PDT
Please also file a follow up on decent tests for this code.
Comment 26 Raymond Lee [:raymondlee] 2013-06-24 18:35:47 PDT
(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] 2013-06-25 07:28:04 PDT
(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] 2013-06-26 18:11:49 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-06-27 08:36:37 PDT
Sure go ahead.
Comment 30 Raymond Lee [:raymondlee] 2013-06-27 08:43:55 PDT
Created attachment 768359 [details] [diff] [review]
Patch for checkin
Comment 31 Ryan VanderMeulen [:RyanVM] 2013-07-01 06:32:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/19845a51b863
Comment 32 Ryan VanderMeulen [:RyanVM] 2013-07-02 12:48:38 PDT
https://hg.mozilla.org/mozilla-central/rev/19845a51b863
Comment 33 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-04 09:49:54 PDT
This is not a documented API, or precisely speaking an API at all.
Comment 34 Jorge Villalobos [:jorgev] 2013-09-23 15:53:03 PDT
This breaks a few add-ons. See bug 919484 for an example.

Note You need to log in before you can comment on or make changes to this bug.