479408 - search engine discovery code doesn't properly handle frames ("browser is null" in BrowserSearch::addEngine)

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Matthew Cline]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.8pre) Gecko/2009021904 GranParadiso/3.0.8pre ID:2009021904

When loading a Japanese Wikipedia page automatically translated to English by Google Translate, I get the following JS error:

Error: browser is null
Source file: chrome://browser/content/browser.js
Line: 2583

It happes at the line:

    if (browser.engines) {

In BrowserSearch::addEngine()

Doesn't happen when viewing the Japanese page without tranlsation, or with  translated Japanese Wikipedia pages other than this bug reports URL.

Comment 1

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

This happens because the page is in a frame, apparently. The search engine discovery code only looks for top-level documents (via getBrowserForDocument()).

Comment 2

[Simon Bünzli]

The same issue also exists in the Feed handler, except that we've got a null check there with the comment // ??? this really shouldn't happen..

What to fix: Both handlers or getBrowserForDocument?

Comment 3

[Phil Ringnalda (:philor)]

If you plan on making discovery of feeds in frames work, I think that'll be the fourth change in our behavior for that without having ever made an explicit decision about how-and-whether (though on the upside, it would be the first *intentional* change) - there's a bug, possibly currently WFM, where Jesse and I had a desultory conversation about the implications.

Comment 4

[Simon Bünzli]

(In reply to comment #3)
That's bug 305472, thanks for the note. Considering the discussion in that bug, I'll stick to updating the comment.

Comment 5

[Simon Bünzli]

Attachment: fix and test

Comment 6

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

Don't the arguments in bug 305472 comment 4 equally apply to search engines? Not sure why they should differ from feeds in this respect.

Comment 7

[Simon Bünzli]

The main difference is that search engines are not as intimately tied to the address bar as the feed icon is.

Comment 8

[Phil Ringnalda (:philor)]

What's that do for data:text/html,<iframe src="http://developer.mozilla.org/"></iframe> when you follow one of the external links to somewhere without a search engine? (And even if it does happen to do the right thing and remove the discovered engine, given our penchant for breaking that, a test would be a good thing).

Do we support discovering multiple engines at once? What happens if the framing document (or even a frameset document) has a search engine and a framed document does too (or multiple framed documents do)? Whoever loads slowest wins?

Comment 9

[Simon Bünzli]

Attachment: fix error and add comments

Right, let's just fix the error, then.

(In reply to comment #8)
> What's that do for data:text/html,<iframe
> src="http://developer.mozilla.org/"></iframe> when you follow one of the
> external links to somewhere without a search engine?

The wrong thing. :-(

> Do we support discovering multiple engines at once?

We do, as long as they've got different titles.

> Whoever loads slowest wins?

First come, first serve.

Comment 10

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

Comment on attachment 363422 [details] [diff] [review]
fix error and add comments

thanks!

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/410bc15d198b

Comment 12

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

Comment on attachment 363422 [details] [diff] [review]
fix error and add comments

a191=beltzner

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/76bf6f2083fa