Marco Bonardo [:mak] Reporter
	Description • 10 years ago

To be able to convert getShortcutOrURIAndPostData to async keywords (that is, to PlacesUtils.promiseHrefAndPostDataForKeyword), we need its callback to be made really asynchronous.

During the conversion to original promises (that were resolving in the same tick) it was onky made fake-async, but it was still synchronous in reality. Then when we moved to Promises.jsm that were resolving on the next tick, it was decided to convert it from promises to a callback due to the high number of tests to fix. It was thus kept synchronous (see bug 989984).

Now, to proceed with the conversion, we need the callback to be really invoked on the next tick, that means we must fix the tests to do the right thing.

this is the tests that are currently failing:

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_getshortcutoruri.js
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_locationBarCommand.js
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbarEnter.js
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbarStop.js
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug1064280_changeUrlInPinnedTab.js
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_search_notification.js
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_uriFixupIntegration.js

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 1 • 10 years ago

Until we have an actual async implementation of getURLAndPostDataForKeyword(), we can at least make getShortcutOrURIAndPostData() behave async with a setTimeout(fn, 0) and fix all failing tests.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 2 • 10 years ago

Fix docshell/test/browser/browser_uriFixupIntegration.js to work with an async getShortcutOrURIAndPostData() and make it work with e10s.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 3 • 10 years ago

Fix browser/base/content/test/general/browser_urlbarStop.js work with an async getShortcutOrURIAndPostData() and make it work with e10s.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 4 • 10 years ago

Fix browser/base/content/test/general/browser_urlbarEnter.js to work with an async getShortcutOrURIAndPostData().

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 10 years ago

Fix browser/base/content/test/general/browser_locationBarCommand.js to work with an async getShortcutOrURIAndPostData().

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 6 • 10 years ago

Will carry it over but didn't want to wait until tomorrow to pick it up...

	:Paolo Amadini
	Comment 7 • 10 years ago

Comment on attachment 8574682 [details] [diff] [review]
0001-Bug-1100291-Make-getShortcutOrURIAndPostData-async-b.patch

I have to recall from memory, but I believe the reason why the callback had to be synchronous wasn't only for tests, but because somewhere some code set a property or called a method on a DOM event object based on the result from the callback.

This wasn't in direct callers but indirectly through other methods that used the function in turn. But I've found mentions of these to be related to the URL bar,  so maybe they've gone away now that we have UnifiedComplete.

Can you please check if no DOM events rely on synchronous behavior?

	Marco Bonardo [:mak] Reporter
	Comment 8 • 10 years ago

We don't have unifiedComplete, the project is blocked due to lack of resources and the awesomebar is still using the old providers.
That said, I don't think the problem is autocomplete search providers, this code runs later, after an entry has been confirmed by the user, by that time the search provider is out of the game.

The problem was tests, but mostly figuring out if those failures in tests were just tests in need of an update, or showing possible regressions that a user may hit.

	:Gijs (he/him)
	Comment 9 • 10 years ago

Comment on attachment 8574683 [details] [diff] [review]
0002-Bug-1100291-Fix-docshell-test-browser-browser_uriFix.patch

Review of attachment 8574683 [details] [diff] [review]:
-----------------------------------------------------------------

This is certainly a lot cleaner and nicer, so r+, but what does this have to do with getShortcutOrURIAndPostData() ?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 10 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #9)
> This is certainly a lot cleaner and nicer, so r+, but what does this have to
> do with getShortcutOrURIAndPostData() ?

It's called by gURLBar.handleCommand() so it fails a few tests that don't expect that action to not immediately kick off a load or similar. Most of those tests don't seem related at first glance.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 11 • 10 years ago

(In reply to :Paolo Amadini from comment #7)
> I have to recall from memory, but I believe the reason why the callback had
> to be synchronous wasn't only for tests, but because somewhere some code set
> a property or called a method on a DOM event object based on the result from
> the callback.

None of the linked bugs say anything about that and I have no idea how to find a DOM event that may rely on such behavior? Ideally that DOM event should be tested and we would thus break a test?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 12 • 10 years ago

Would hypnotizing you help remember what DOM event that was? Any other less invasive ideas? :)

	:Paolo Amadini
	Comment 13 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #12)
> Would hypnotizing you help remember what DOM event that was? Any other less
> invasive ideas? :)

That could work, but looking at the current code might work as well:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#363

Those wouldn't have any effect if called asynchronously. As the comment says, they may actually be unnecessary, please verify if this is the case and just remove the code. I think the original callers are ontextentered on the URL bar textbox and onclick on the "go" button.

In the same function, there are various suspect lines like "gBrowser.userTypedValue = url;" that would become asynchronous as well. I don't know whether this can raise race conditions or security concerns - Marco certainly can know more.

I've checked the other code paths and it seems that the event object is passed around only for getting information about which tab or window to use, so that should be fine.

	Dão Gottwald [:dao]
	Comment 14 • 10 years ago

Comment on attachment 8574686 [details] [diff] [review]
0004-Bug-1100291-Fix-browser-base-content-test-general-br.patch

>+add_task(function* () {
>+  info("Simple return keypress");
>   let tab = gBrowser.selectedTab = gBrowser.addTab(START_VALUE);
>-  addPageShowListener(function() {
>-    locationBarEnter(test.event, function() {
>-      test.check(tab);
> 
>-      // Clean up
>-      while (gBrowser.tabs.length > 1)
>-        gBrowser.removeTab(gBrowser.selectedTab)
>-      runNextTest();
>-    });
>-  });
>-}
>+  gURLBar.focus();
>+  EventUtils.synthesizeKey("VK_RETURN", {});
>+  yield BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
>+
>+  // Check url bar and selected tab.
>+  is(gURLBar.textValue, TEST_VALUE, "Urlbar should preserve the value on return keypress");
>+  is(gBrowser.selectedTab, tab, "New URL was loaded in the current tab");
> 
>-let gTests = [
>-  { desc: "Simple return keypress",
>-    event: {},
>-    check: checkCurrent
>-  },
>+  // Cleanup.
>+  gBrowser.removeCurrentTab();
>+});

The removeCurrentTab call seems wrong, as you didn't add a tab beforehand, did you?

Was it really necessary to rewrite the whole test? :/

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 15 • 10 years ago

Comment on attachment 8574686 [details] [diff] [review]
0004-Bug-1100291-Fix-browser-base-content-test-general-br.patch

(In reply to Dão Gottwald [:dao] from comment #14)
> The removeCurrentTab call seems wrong, as you didn't add a tab beforehand,
> did you?

So... the first test does add a new tab and expects the load to happen in that newly opened tab. At the end of the test it's removed.

The second test adds a new tab and expects the load to happen in yet another tab (Alt+Return). Both of those tabs are closed at the end of the test.

> Was it really necessary to rewrite the whole test? :/

I mean, no, it's probably not *necessary*. But if I spent time already on trying to understand the whole test once why not rewrite it to make it easier for people after me to understand what it's doing?

	Dão Gottwald [:dao]
	Comment 16 • 10 years ago

Comment on attachment 8574686 [details] [diff] [review]
0004-Bug-1100291-Fix-browser-base-content-test-general-br.patch

Apparently I missed the addTab call.

	Dão Gottwald [:dao]
	Comment 17 • 10 years ago

Comment on attachment 8574684 [details] [diff] [review]
0003-Bug-1100291-Make-browser-base-content-test-general-b.patch

>+  yield BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
>   is(gURLBar.textValue, gURLBar.trimValue(goodURL), "location bar reflects loaded page");
> 
>-  typeAndSubmit(badURL);
>-  is(gURLBar.textValue, gURLBar.trimValue(badURL), "location bar reflects loading page");
>-
>-  gBrowser.contentWindow.stop();
>+  yield typeAndSubmitAndStop(badURL);
>   is(gURLBar.textValue, gURLBar.trimValue(goodURL), "location bar reflects loaded page after stop()");

Removing the "location bar reflects loading page" check doesn't seem like a good idea. If some day we break this part, the "location bar reflects loaded page after stop()" check becomes pointless too and the test hardly tests anything.

	:Margaret Leibovic
	Comment 18 • 9 years ago

Comment on attachment 8574687 [details] [diff] [review]
0005-Bug-1100291-Fix-browser-base-content-test-general-br.patch

Review of attachment 8574687 [details] [diff] [review]:
-----------------------------------------------------------------

This logic is tricky, good thing we have a test! There were a lot of changes here, so it was a bit tricky to follow the original testcases moving over into this new test file structure, but I think overall this test is more readable now.

::: browser/base/content/test/general/browser_locationBarCommand.js
@@ +194,4 @@
>    });
>  }
>  
> +function waitForNewWindow() {

I think putting "promise" at the beginning of this function name would make it more consistent with the other helper functions in here.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 19 • 9 years ago

(In reply to Dão Gottwald [:dao] from comment #17)
> Removing the "location bar reflects loading page" check doesn't seem like a
> good idea.

Good catch, I don't remember explicitly deciding to remove those so maybe they just got lost in translation.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 20 • 9 years ago

(In reply to :Paolo Amadini from comment #13)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> urlbarBindings.xml#363
> 
> Those wouldn't have any effect if called asynchronously. As the comment
> says, they may actually be unnecessary, please verify if this is the case
> and just remove the code. I think the original callers are ontextentered on
> the URL bar textbox and onclick on the "go" button.

Those lines were added without a bug number or justification ~13 years ago:

https://github.com/ttaubert/gecko-dev/commit/5bf459706fcc033d0a27d0d155e042a3bb54c0d1

Please note that the variable name is |event| here but should be |aTriggeringEvent|. That typo was fixed ~3 years after the original landing:

https://github.com/ttaubert/gecko-dev/commit/12a56b93b4d084d1a0727ff8aae1ae858f950a76

That seems to indicate that not actually calling .preventDefault() and .stopPropagation() didn't seem to have any effects other than the JS error in the console. I would vote for removing those. If it turns out that there's a beep or any other weird default action we could probably put it back in and move it out of continueOperation().

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 21 • 9 years ago

(In reply to :Paolo Amadini from comment #13)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> urlbarBindings.xml#363
> 
> In the same function, there are various suspect lines like
> "gBrowser.userTypedValue = url;" that would become asynchronous as well. I
> don't know whether this can raise race conditions or security concerns -
> Marco certainly can know more.

Looking at the code I wouldn't think this could case bad things but Marco certainly knows better.

	:Paolo Amadini
	Comment 22 • 9 years ago

Comment on attachment 8577977 [details] [diff] [review]
0001-Bug-1100291-Make-getShortcutOrURIAndPostData-async-b.patch, v2

Thanks for checking.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 24 • 9 years ago

Looking good with those last test fixes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d018fab5fc65

(Apart from the infra 503s that is :)

	:Gijs (he/him)
	Comment 25 • 9 years ago

Comment on attachment 8578761 [details] [diff] [review]
0006-Bug-1100291-Fix-a-few-tests-to-wait-until-a-newly-op.patch

Review of attachment 8578761 [details] [diff] [review]:
-----------------------------------------------------------------

I feel like I don't understand this well enough to give r+. Can you clarify/revise? :-)

::: browser/base/content/test/general/browser_bug1064280_changeUrlInPinnedTab.js
@@ +10,5 @@
>    let TEST_LINK_CHANGED = "about:support";
>  
>    let appTab = gBrowser.addTab(TEST_LINK_INITIAL);
> +  let browser = appTab.linkedBrowser;
> +  yield BrowserTestUtils.browserLoaded(browser);

Why is the initial yield here necessary?

::: browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js
@@ +59,5 @@
>    ]);
>  }
>  
>  add_task(function* test_navigate_full_domain() {
> +  let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");

Why does the URL need to be explicit where it wasn't before? And why about:blank rather than about:newtab (which I'm guessing is what addTab will do without a URL being passed)?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 26 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #25)
> >    let appTab = gBrowser.addTab(TEST_LINK_INITIAL);
> > +  let browser = appTab.linkedBrowser;
> > +  yield BrowserTestUtils.browserLoaded(browser);
> 
> Why is the initial yield here necessary?

So the problem is that .handleCommand() used to immediately stop the current load and start a new one. As it's "async" now that might happen a tad later. All the tests fixed here add a new tab but don't wait for the initial load to finish. waitForDocLoadAndStopIt() and other functions thus catch the initial load but not the one caused by gURLBar.handleCommand().

> >  add_task(function* test_navigate_full_domain() {
> > +  let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
> 
> Why does the URL need to be explicit where it wasn't before? And why
> about:blank rather than about:newtab (which I'm guessing is what addTab will
> do without a URL being passed)?

The URL doesn't need to be explicit. I assume though that the test author didn't actually want about:newtab but just a blank tab and I thought it would be nice to use that. Continuing to use about:newtab wouldn't really hurt though.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 27 • 9 years ago

Comment on attachment 8578761 [details] [diff] [review]
0006-Bug-1100291-Fix-a-few-tests-to-wait-until-a-newly-op.patch

Please see comment #26.

	:Gijs (he/him)
	Comment 28 • 9 years ago

Comment on attachment 8578761 [details] [diff] [review]
0006-Bug-1100291-Fix-a-few-tests-to-wait-until-a-newly-op.patch

Review of attachment 8578761 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanations!

	Marco Bonardo [:mak] Reporter
	Comment 29 • 9 years ago

Comment on attachment 8577977 [details] [diff] [review]
0001-Bug-1100291-Make-getShortcutOrURIAndPostData-async-b.patch, v2

Review of attachment 8577977 [details] [diff] [review]:
-----------------------------------------------------------------

I think it's OK. thank you for taking the time to test this.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 30 • 9 years ago

Dão, gentle review ping? Would be great to land this before the patches bitrot :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 31 • 9 years ago

Thanks everyone!

https://hg.mozilla.org/integration/fx-team/rev/5727e5db4bb1
https://hg.mozilla.org/integration/fx-team/rev/4756d973a34c
https://hg.mozilla.org/integration/fx-team/rev/51ca14f617a6
https://hg.mozilla.org/integration/fx-team/rev/cd52056697d5
https://hg.mozilla.org/integration/fx-team/rev/cccba602c83a
https://hg.mozilla.org/integration/fx-team/rev/2894f27c8ca7

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/5727e5db4bb1
https://hg.mozilla.org/mozilla-central/rev/4756d973a34c
https://hg.mozilla.org/mozilla-central/rev/51ca14f617a6
https://hg.mozilla.org/mozilla-central/rev/cd52056697d5
https://hg.mozilla.org/mozilla-central/rev/cccba602c83a
https://hg.mozilla.org/mozilla-central/rev/2894f27c8ca7