Closed Bug 1096534 Opened 10 years ago Closed 9 years ago

ContentSearch should load the search URL in the browser/tab sending the search message, not the current/selected browser in the top-level chrome window

Categories

(Firefox :: Search, defect)

33 Branch
defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.1
Tracking Status
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified

People

(Reporter: adw, Assigned: adw, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1093571 +++

On about:newtab:

1. Choose a search engine that provides suggestions, like Google
2. In the search box, quickly type gibberish and press enter
3. Quickly open a new tab (Ctrl/Cmd+T)

If you're quick enough and the search suggestions take long enough to come in, then the search results URL loads in the newly opened tab.  It should load in the about:newtab tab.

ContentSearch._onMessageSearch has access to the xul:browser that requested the search (via msg.target), but instead it gets the top-level chrome window and uses it to load the URL (browserWin.loadURI).  That's dumb.  It should call loadURI on msg.target or something equivalent instead.  This should be pretty simple to fix.

http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=6818a0733662#208
Flags: firefox-backlog+
Flags: qe-verify+
Blocks: 1028985
Blocks: 962490
No longer blocks: 1028985
Blocks: 1028985
Attached patch patch (obsolete) — Splinter Review
This is an embarrassing bug with a quick fix so I hope it's OK that I worked on it.

In case it's not clear, the reason for the `loaded` bool is to avoid silently catching errors thrown by recordSearchInHealthReport.  We definitely want to spot those.

This patch also fixes a JS strict warning introduced by bug 1036908:

JavaScript strict warning: resource:///modules/ContentSearch.jsm, line 211: ReferenceError: reference to undefined property data.selection
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attachment #8528525 - Flags: review?(mak77)
Iteration: --- → 37.1
Comment on attachment 8528525 [details] [diff] [review]
patch

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

I'd usually request a test (or to modify the existing test making it open a new tab when a search starts), but looks like in this case it may not be easily predictable the order in which open tab and search messages are handled in e10s mode?

::: browser/modules/ContentSearch.jsm
@@ +219,5 @@
> +      // message and the time that we received it.  In that case, trying to call
> +      // any method on it will throw.
> +    }
> +    if (loaded) {
> +      let browser = msg.target.ownerDocument.defaultView;

nit: Just for code clarity, you could let browser = msg.target; at the top, and let win = browser.ownerDoc... here

@@ +222,5 @@
> +    if (loaded) {
> +      let browser = msg.target.ownerDocument.defaultView;
> +      browser.BrowserSearch.recordSearchInHealthReport(engine, data.whence,
> +                                                       data.selection || null);
> +    }

I think it's simpler if you early "return Promise.resolve();" in the above catch... then you can avoid the loaded boolean.
Attachment #8528525 - Flags: review?(mak77) → review+
Thanks, Marco.  Landed with a test and comments addressed.

https://hg.mozilla.org/integration/fx-team/rev/945080e133d2
Attachment #8528525 - Attachment is obsolete: true
Attachment #8533273 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/945080e133d2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
QA Contact: petruta.rasa
Comment on attachment 8533273 [details] [diff] [review]
checked-in patch with test

Approval Request Comment
[Feature/regressing bug #]:
Bug 962490, search box in about:newtab

[User impact if declined]:
This bug has been present since 31, but it became much easier to trigger after search suggestions were added to about:newtab in 34 (bug 1028985).  So so far, this bug has been easy to trigger in only one release.  If we let the fix ride the trains instead of uplifting it, the bug will be easy to trigger in the 35 and 36 releases, too.

[Describe test coverage new/current, TBPL]:
The fix comes with a new browser-chrome test.  Searching on about:newtab was already covered by existing related automated tests.

[Risks and why]:
Moderate risk given that this touches code related to searching on about:newtab, but the risk is mitigated by automated test coverage.  I ran the new test against both Aurora and Beta before requesting uplift approval.

[String/UUID change made/needed]:
None
Attachment #8533273 - Flags: approval-mozilla-beta?
Attachment #8533273 - Flags: approval-mozilla-aurora?
Attachment #8533273 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8533273 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reproduced using Firefox 34.0 and DuckDuckGo as default search engine.

Verified as fixed using Firefox 35 beta 3, latest Dev Edition 36.0a2 and latest Nightly 37.0a1 2014-12-12 under Win 7 64-bit, Ubuntu 14.04 32-bit and Mac OSX 10.9.5.
You need to log in before you can comment on or make changes to this bug.