706398 - "gSearchEngine is null" when about:home is closed early

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

about:home throws 'gSearchEngine is null' when opened in a new tab that gets closed directly after the load event. This happens in some Panorama tests.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Trivial patch checking for gSearchEngine.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 577877 [details] [diff] [review]
patch v1

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

Where does it notifies that exactly? I'd prefer to handle the actual location rather than conditioning the whole method, or finding a better way to avoid onLoad if the page is being closed.
The only location I found that used gSearchEngine immediately is inside setupSearchEngine where the first command is gSearchEngine = JSON.parse(localStorage["search-engine"]); is it this line?
So, the page is being destroyed (gSearchEngine has already been destroyed) but the onLoad handler is still running? Sounds like bad enough to understand if we are handling onload the wrong way...

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [:mak] from comment #2)
> The only location I found that used gSearchEngine immediately is inside
> setupSearchEngine where the first command is gSearchEngine =
> JSON.parse(localStorage["search-engine"]); is it this line?

It just says window:0 or something like that. Seems it's not able to determine the failing line but with the condition fixing this problem it must be that line you pointed out.

> So, the page is being destroyed (gSearchEngine has already been destroyed)
> but the onLoad handler is still running? Sounds like bad enough to
> understand if we are handling onload the wrong way...

Yeah, true. Sorry for my quick symptom fix. I'll attach a test case.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: test case

Test case causing the error.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

That's the line causing the error:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutHome.js#184

Comment 6

[Marco Bonardo [:mak]]

ah, so localStorage["search-engine"] is null, JSON.parse(null) is null, and we are actually doing gSearchEngine = null;

Comment 7

[Marco Bonardo [:mak]]

that likely happens due to how the browser chrome harness opens the browser window, since get defaultArgs() is likely not invoked (I suppose the browser is opened with a location in the command line.

Would be fine to bail out if gSearchEngine is null after the JSON.parse, for now. We should change the implementation to use something better than domstorage to communicate these data from chrome to content.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Checks if gSearchEngine is null in setupSearchEngine().

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 578603 [details] [diff] [review]
patch v2

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

r=me with the below addressed

::: browser/base/content/aboutHome.js
@@ +201,1 @@
>    }

may you rather invert the check and do

if (!gSearchEngine)
  return;

so we preserve the blame

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/f40ab132ed31

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f40ab132ed31