551434 - Toolbar search (search bar / keywords) work not in selected engine but in active page

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Danil Ilinykh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)

Active page with suggested search plugins prevent search from search toolbar.

Reproducible: Always

Steps to Reproduce:
1. Go to http://slovari.yandex.ru/
2. Select "Google" in search toolbar.
3. Type "hello".
4. Press Enter.
Actual Results:  
Loading page: http://slovari.yandex.ru/search.xml?text=&st_translate=on

Expected Results:  
Loaded page: http://www.google.com/search?q=hello&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:ru:official&client=firefox

Alt+Enter works good

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Good catch. This is a regression from Firefox 3.5. Confirmed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.2pre) Gecko/20100308 Namoroka/3.6.2pre ID:20100308035343

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

on trunk i get as actual behavior http://slovari.yandex.ru/search.xml?text=&st_translate=0 loaded + a new tab opened with the google search for the search term. weird.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Even with a fresh profile? When I run a trunk build no new tabs will be opened.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Looks like the page just steals focus on Enter. A regression range would be helpful...

Comment 5

[Alice0775 White]

regression window:
works:
http://hg.mozilla.org/mozilla-central/rev/90d3e6d2cbb9
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090610 Namoroka/3.6a1pre ID:20090610042525

fails:
http://hg.mozilla.org/mozilla-central/rev/4430cae50dad
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090611 Namoroka/3.6a1pre ID:20090611044033

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90d3e6d2cbb9&tochange=4430cae50dad

Comment 6

[Alice0775 White]

WORKAROUND:

diff -b -U 20 org/utilityOverlay.js new/utilityOverlay.js
--- org/utilityOverlay.js	2010-03-06 15:52:16.569609600 +0900
+++ new/utilityOverlay.js	2010-03-20 03:07:30.956487600 +0900
@@ -235,41 +235,41 @@
   case "tabshifted":
     loadInBackground = !loadInBackground;
     // fall through
   case "tab":
     let browser = w.gBrowser;
     browser.loadOneTab(url, {
                        referrerURI: aReferrerURI,
                        postData: aPostData,
                        inBackground: loadInBackground,
                        allowThirdPartyFixup: aAllowThirdPartyFixup,
                        relatedToCurrent: aRelatedToCurrent});
     break;
   }
 
   // If this window is active, focus the target window. Otherwise, focus the
   // content but don't raise the window, since the URI we just loaded may have
   // resulted in a new frontmost window (e.g. "javascript:window.open('');").
   var fm = Components.classes["@mozilla.org/focus-manager;1"].
              getService(Components.interfaces.nsIFocusManager);
   if (window == fm.activeWindow)
-    w.content.focus();
+    setTimeout(function(w){w.content.focus();},0);
   else
     w.gBrowser.selectedBrowser.focus();
 }
 
 // Used as an onclick handler for UI elements with link-like behavior.
 // e.g. onclick="checkForMiddleClick(this, event);"
 function checkForMiddleClick(node, event) {
   // We should be using the disabled property here instead of the attribute,
   // but some elements that this function is used with don't support it (e.g.
   // menuitem).
   if (node.getAttribute("disabled") == "true")
     return; // Do nothing
 
   if (event.button == 1) {
     /* Execute the node's oncommand or command.
      *
      * XXX: we should use node.oncommand(event) once bug 246720 is fixed.
      */
     var target = node.hasAttribute("oncommand") ? node :
                  node.ownerDocument.getElementById(node.getAttribute("command"));

Comment 7

[Alice0775 White]

In addition to the Comment #0,
Keyword search on location bar also broken.

Steps to Reproduce:
1. Go to http://slovari.yandex.ru/
2. Wait complete loading the page
3. Type "Mozilla Firefox" in Locationbar (keyword.URL should be default value, thus google search)
4. Press Enter.

Actual Results:  
Loading page: http://slovari.yandex.ru/search.xml?text=&st_translate=0

Expected Results:  
Loaded page:
http://www.mozilla.com/en-US/firefox/firefox.html
OR
http://www.google.com/search?ie=UTF-8&oe=UTF-8&sourceid=navclient&gfns=1&q=Mozilla+Firefox

This also something wrong  :
gBrowser.selectedBrowser.focus(); in gURLBar.handleCommand .

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100319 Minefield/3.7a4pre ID:20100319040017

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Neil, would you have time to check this bug?

Comment 10

[mayestera]

I can not reproduce the original bug with the URL http://slovari.yandex.ru/ with version 3.6.3, it seems that it works fine (also the location bar). Fails with the address from the duplicated bug http://www.terra.es

Comment 11

[Alice0775 White]

(In reply to comment #10)
> I can not reproduce the original bug with the URL http://slovari.yandex.ru/
> with version 3.6.3, it seems that it works fine (also the location bar). Fails
> with the address from the duplicated bug http://www.terra.es

I can not reproduce the URL http://slovari.yandex.ru/ on  Namoroka/3.6a1pre ID:20090611044033 too.
It means the site changed something .

Comment 12

[Alice0775 White]

Attachment: testcase

[STR1]
1. Start Fiirefox with new profile
2. Open testcase
3. Input text in the SearchBar and Press Enter key

[STR2]
1. Start Fiirefox with new profile
2. Open testcase
3. Input text in the LocationBar and Press Enter key

[Actual resalt]
Loading  https://bugzilla.mozilla.org/

[Expected]
Search results are displayed

Comment 16

[Damon Sicore (:damons)]

Neil, can you take a look at this?  Also, bumping this to beta2 per beta re-triage with dbaron.  If this should indeed block beta1, please re-nom.

Comment 17

[Neil Deakin]

This testcase loads a url whenever it receives focus.

openUILinkIn loads the search url and then focuses the content window. Should it be waiting for the new url to start loading before it does this?

Comment 18

[Alice0775 White]

(In reply to comment #17)
> This testcase loads a url whenever it receives focus.
Yes.
> 
> openUILinkIn loads the search url and then focuses the content window. Should
> it be waiting for the new url to start loading before it does this?
 Yes that is problem, and this behavior was brought by fixing bug 178324.

I think that the focus command should be performed after finish new page loading.

Comment 19

[Dão Gottwald [:dao]]

The testcase seems unrealistic and not really relevant for real-world pages.

http://www.terra.es/ listens for the keyup event, which we clearly don't want it to get when you press enter in the search bar...

Comment 20

[Neil Deakin]

(In reply to comment #19)
> http://www.terra.es/ listens for the keyup event, which we clearly don't want
> it to get when you press enter in the search bar...

That is bug 552285. Is this one supposed to be a the same issue?

Comment 21

[Dão Gottwald [:dao]]

Attachment: testcase

more realistic testcase based on http://www.terra.es/

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #20)
> (In reply to comment #19)
> > http://www.terra.es/ listens for the keyup event, which we clearly don't want
> > it to get when you press enter in the search bar...
> 
> That is bug 552285. Is this one supposed to be a the same issue?

Appears to be the same.

Comment 23

[Alice0775 White]

There are two problems here,
1. focus event fire to content before complete loading page
2. focus event fire to content before keyup event to serchbar/urlbar,
aren't it?

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #23)
> There are two problems here,
> 1. focus event fire to content before complete loading page

Sure, but focusing web pages isn't generally a problem. We do this when switching tabs, for instance, which causes your test case to navigate away as well. This test case is just quite weird.

> 2. focus event fire to content before keyup event to serchbar/urlbar,
> aren't it?

Yes, that's the real problem. We don't want content to get this keyup event.

Comment 25

[Alice0775 White]

(In reply to comment #24)
> (In reply to comment #23)
> > There are two problems here,
> > 1. focus event fire to content before complete loading page
> 
> Sure, but focusing web pages isn't generally a problem. We do this when
> switching tabs, for instance, which causes your test case to navigate away as
> well. This test case is just quite weird.

Yes, the testcase is not real.
However, in the release build,
1.surplus focus event fired to existing content page. -- something bad behavior
2.not fire focus event  to  new loaded page.-- should be fixed

I think that both should be fixed.

Comment 26

[Dão Gottwald [:dao]]

> 2.not fire focus event  to  new loaded page.-- should be fixed

You get the same behavior when clicking a link which replaces the current page with a new page.

Comment 27

[Alice0775 White]

(In reply to comment #26)
> > 2.not fire focus event  to  new loaded page.-- should be fixed
> 
> You get the same behavior when clicking a link which replaces the current page
> with a new page.

I see.

Comment 28

[Neil Deakin]

Attachment: One possibility is to store the target of the keydown and fire keypress and keyup at the same target

Note that this version of the patch still leaks the stored node.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

This needs to be fixed ASAP. Visible regression on branch.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Olli? You working on this? This is a hard beta 3 blocker, and I think we should try to get it into 1.9.2 ASAP despite only being marked as "wanted", it's a bad and visible regression.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Somehow I don't think this is going to make it into Beta 3. I'd like to know what to do to get more motion/traction on this bug. Damon, any ideas?

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

How do we ensure this doesn't slip another beta?

Comment 34

[Max]

Surely this has security implications as well? eg.

1. Go to dodgy website
2. Search for something using toolbar search
3. Search query instead gets passed to dodgy website
4. Dodgy website then returns a spoof search page, eg. a page that looks like Google, with malicious links.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Sorry, I wasn't CC'ed to this bug so I didn't get any bugmail, except the
feedback email long ago. (And since it is not in my review queue, I somehow
managed to not notice it)

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 455744 [details] [diff] [review]
One possibility is to store the target of the keydown and fire keypress and keyup at the same target

This looks pretty regression risky, and has problems.
What happens if the target of the keydown is removed from document
before keypress? Or what if the element is moved to another document?
Focusmanager thinks that the focus is still in the old window, but
key events would go to some other window.

What is wrong with the approach in comment 6?

Comment 37

[Neil Deakin]

(In reply to comment #36)

> What is wrong with the approach in comment 6?

Because it doesn't work. For the workaround given in comment 6, if you'll notice closely, 'w' will be null, so an error just occurs, focusing nothing.

-    w.content.focus();
+    setTimeout(function(w){w.content.focus();},0);

A related workaround may be to focus after the document has started loaded and appeared onscreen, as I described in comment 17. But Dao or Gavin might be better at knowing whether that was viable.

Comment 38

[Dão Gottwald [:dao]]

(In reply to comment #37)
> A related workaround may be to focus after the document has started loaded and
> appeared onscreen, as I described in comment 17. But Dao or Gavin might be
> better at knowing whether that was viable.

This sounds fragile, as the user could do other stuff in the meantime. If somehow viable I still think making content not get the keyup event for key strokes started in chrome would be the ideal solution, rather than hacking around it every time this issue might pop up in chrome.

Comment 39

[Max]

How do other browsers handle this issue? Surely that would be the way forward?

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Other browsers don't have DOM in chrome and content.

I think I could accept approach where the key event target can't change
between keydown and keyup if the original target is in chrome and the possible
new target in content. That should be less-regression-risky approach.
That would be some rather small tweaks to the patch.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Changing the keyboard target handling in content pages is very regression risky.
I believe Google uses all sort of hack for IME etc. to move the focus in
keydown.

Comment 42

[Neil Deakin]

Attachment: patch

This patch stores the keydown target and only retargets the keypress/keyup events when focus changes from chrome to content.

However, this patch leaks if shutdown with Ctrl/Cmd+Q and I'm not sure why. gKeyDownTarget is set to null at shutdown, yet it doesn't leak if gKeyDown is never set.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

What all does the patch leak? Only the element or "the world"?

Comment 44

[Neil Deakin]

Not just the element. Everything leaks.

Comment 45

[Dietrich Ayala (:dietrich)]

Neil's still chasing the leak, and says it's unlikely to be resolved in time for the freeze for b4. Not going to change the flag yet, but making a note so drivers are aware of the risk of slippage here.

Comment 46

[Neil Deakin]

Attachment: patch which doesn't leak

Comment 47

[Daniel Veditz [:dveditz]]

I'm not sure why this was made a 1.9.2 release blocker if it's been affecting us the series. A fix is nice, sure, but why not just take it whenever it's done for trunk?

Comment 48

[LegNeato]

Removing the blocking flag for 1.9.2.9, as it wouldn't bock the release (we've lived with it this long in 3.6.x). We'll pick this up in the next one, assuming the review goes through and it gets nominated again.

Comment 49

[Mike Beltzner [:beltzner, not reading bugmail]]

FWIW, we added the 1.9.2.9 flag because recent changes in the code on the websites (google and linkedin) which exercised the bug.

Comment 50

[Mike Beltzner [:beltzner, not reading bugmail]]

Looks like we got a patch, should it be checked in? Are we waiting on a talos run from tryserver or something?

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

I've asked Gavin to run this through tryserver so we can make a call tomorrow morning on how safe it is to take at the last moment.

Comment 52

[Mike Beltzner [:beltzner, not reading bugmail]]

--> beta5, we'll get it early next beta cycle, like right after the tree re-opens.

Comment 53

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f43f9b764efb

Target milestone is mozilla2.0b5.

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre

Comment 55

[Szabolcs Horvát]

Is it planned to include this fix in the 3.6 series?  

I understand that most people don't use websites that are affected by this problem, but for those of us who visit such sites several times a day (e.g. http://nciku.com/), this bug is highly annoying.

Comment 57

[Brandon Sterne (:bsterne)]

It would be great to get a 1.9.2 patch for this bug.  We have had traffic to the security@m.o list regarding seeing this bug in the wild. Neil, any chance you could make a branch patch for us?

Comment 58

[LegNeato]

This patch doesn't apply cleanly, but the backporting looks fairly trivial. Attaching an untested 1.9.2 version.

Comment 59

[LegNeato]

Attachment: Possible 1.9.2 patch

Comment 60

[Neil Deakin]

Attachment: Working 1.9.2 patch which runs the test

I'm wondering if we should be retargeting to the right presshell on trunk as well.

Comment 61

[LegNeato]

Moving to 1.9.2.15 as this missed 1.9.2.14 and it is sg:low

Comment 62

[Daniel Veditz [:dveditz]]

Comment on attachment 505809 [details] [diff] [review]
Working 1.9.2 patch which runs the test

Approved for 1.9.2.17, a=dveditz for release-drivers

Comment 63

[Neil Deakin]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/59e645275feb