Closed Bug 1100291 Opened 5 years ago Closed 5 years ago

getShortcutOrURIAndPostData callback should be asynchronous

Categories

(Firefox :: General, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox39 --- fixed

People

(Reporter: mak, Assigned: ttaubert)

References

Details

Attachments

(6 files, 2 obsolete files)

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
Flags: qe-verify-
Flags: firefox-backlog+
Blocks: 1100294
Blocks: 1093941
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.
Attachment #8574682 - Flags: review?(paolo.mozmail)
Fix docshell/test/browser/browser_uriFixupIntegration.js to work with an async getShortcutOrURIAndPostData() and make it work with e10s.
Attachment #8574683 - Flags: review?(gijskruitbosch+bugs)
Fix browser/base/content/test/general/browser_urlbarStop.js work with an async getShortcutOrURIAndPostData() and make it work with e10s.
Attachment #8574684 - Flags: review?(dao)
Fix browser/base/content/test/general/browser_urlbarEnter.js to work with an async getShortcutOrURIAndPostData().
Attachment #8574686 - Flags: review?(dao)
Fix browser/base/content/test/general/browser_locationBarCommand.js to work with an async getShortcutOrURIAndPostData().
Attachment #8574687 - Flags: review?(margaret.leibovic)
Will carry it over but didn't want to wait until tomorrow to pick it up...
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Points: 3 → 5
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?
Attachment #8574682 - Flags: review?(paolo.mozmail)
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.
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() ?
Attachment #8574683 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
(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?
Would hypnotizing you help remember what DOM event that was? Any other less invasive ideas? :)
Flags: needinfo?(paolo.mozmail)
Blocks: 1141547
(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.
Flags: needinfo?(paolo.mozmail)
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
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? :/
Attachment #8574686 - Flags: review?(dao) → review-
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?
Attachment #8574686 - Flags: review- → review?(dao)
Comment on attachment 8574686 [details] [diff] [review]
0004-Bug-1100291-Fix-browser-base-content-test-general-br.patch

Apparently I missed the addTab call.
Attachment #8574686 - Flags: review?(dao) → review+
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.
Attachment #8574684 - Flags: review?(dao) → review-
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.
Attachment #8574687 - Flags: review?(margaret.leibovic) → review+
(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.
Attachment #8574684 - Attachment is obsolete: true
Attachment #8577969 - Flags: review?(dao)
(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().
(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.
Attachment #8574682 - Attachment is obsolete: true
Attachment #8577977 - Flags: review?(paolo.mozmail)
Attachment #8577977 - Flags: feedback?(mak77)
Comment on attachment 8577977 [details] [diff] [review]
0001-Bug-1100291-Make-getShortcutOrURIAndPostData-async-b.patch, v2

Thanks for checking.
Attachment #8577977 - Flags: review?(paolo.mozmail) → review+
Looking good with those last test fixes:

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

(Apart from the infra 503s that is :)
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)?
Attachment #8578761 - Flags: review?(gijskruitbosch+bugs)
(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.
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.
Attachment #8578761 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8578761 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8577977 - Flags: feedback?(mak77) → feedback+
Dão, gentle review ping? Would be great to land this before the patches bitrot :)
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Attachment #8577969 - Flags: review?(dao) → review+
Depends on: 1147720
You need to log in before you can comment on or make changes to this bug.