Use asynchronous getCharsetForURI in getShortcutOrURI

RESOLVED FIXED in mozilla25

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

(Blocks: 2 bugs, {addon-compat})

Trunk
mozilla25
x86
Mac OS X
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2274
(Assignee)

Updated

5 years ago
Assignee: nobody → raymond
(Assignee)

Comment 1

5 years ago
Created attachment 720263 [details] [diff] [review]
v1
Attachment #720263 - Flags: review?(mak77)

Updated

4 years ago
No longer blocks: 834543
Depends on: 834543

Updated

4 years ago
Blocks: 699850
Attachment #720263 - Flags: review?(mak77) → review?(mano)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Mano: do you have a chance to review this patch? thanks
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.
Attachment #720263 - Flags: review?(mano) → review-
(Assignee)

Comment 4

4 years ago
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.
Attachment #720263 - Attachment is obsolete: true
Attachment #736816 - Flags: review?(mano)
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.
Attachment #736816 - Flags: review?(mano) → review-
(Assignee)

Comment 6

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

Comment 8

4 years ago
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.
Attachment #738961 - Flags: review?(mano)
(Assignee)

Updated

4 years ago
Attachment #736816 - Attachment is obsolete: true
(Assignee)

Comment 9

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

@mano: please let me know what this patch looks ok.  Thanks!
I'll get to this patch in the next couple of days. Sorry this is taking so long.
Reviewing this now finally. Sorry again. Raymond, any chance you could also attach the full diff (without -w, that is)?
(Assignee)

Comment 12

4 years ago
Created attachment 757449 [details] [diff] [review]
v3 without -w

Here is the without -w version
Attachment #757449 - Flags: review?(mano)
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?
By the way, if you're ever on IRC (moznet), please message me.
(Assignee)

Comment 15

4 years ago
(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.
Flags: needinfo?(mano)
(Assignee)

Updated

4 years ago
Blocks: 854925
(Assignee)

Updated

4 years ago
Blocks: 880922
(Assignee)

Updated

4 years ago
No longer blocks: 854925
(Assignee)

Updated

4 years ago
Attachment #738961 - Flags: review?(mano)
(Assignee)

Updated

4 years ago
Attachment #757449 - Flags: review?(mano)
Why did you remove the flags?

(Mano on IRC)
Flags: needinfo?(mano)
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.
> All the code which sets |lastURL| should be added to tabbrowser.xml, right?

Yes.
(Assignee)

Comment 19

4 years ago
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.
Attachment #757449 - Attachment is obsolete: true
Attachment #761906 - Flags: review?(mano)
(Assignee)

Comment 20

4 years ago
Created attachment 761908 [details] [diff] [review]
v4 -w
Attachment #738961 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #761908 - Attachment is patch: true
Attachment #761908 - Attachment mime type: text/x-patch → text/plain
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.
(Assignee)

Comment 22

4 years ago
Created attachment 762474 [details] [diff] [review]
v5
Attachment #761906 - Attachment is obsolete: true
Attachment #761906 - Flags: review?(mano)
Attachment #762474 - Flags: review?(mano)
(Assignee)

Comment 23

4 years ago
Created attachment 762476 [details] [diff] [review]
v5 -w
Attachment #761908 - Attachment is obsolete: true
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.
Attachment #762474 - Flags: review?(mano) → review+
Please also file a follow up on decent tests for this code.
(Assignee)

Updated

4 years ago
Blocks: 886639
(Assignee)

Comment 26

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

Filed bug 886639
(Assignee)

Comment 27

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

Comment 28

4 years ago
(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?
Sure go ahead.
(Assignee)

Comment 30

4 years ago
Created attachment 768359 [details] [diff] [review]
Patch for checkin
Attachment #762474 - Attachment is obsolete: true
Attachment #762476 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/19845a51b863
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/19845a51b863
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Keywords: dev-doc-needed
This is not a documented API, or precisely speaking an API at all.
Keywords: dev-doc-needed
This breaks a few add-ons. See bug 919484 for an example.
Keywords: addon-compat
Depends on: 1000458
Depends on: 1044351

Updated

3 years ago
Attachment #757449 - Attachment is patch: true
Attachment #757449 - Attachment mime type: text/x-patch → text/plain
You need to log in before you can comment on or make changes to this bug.