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

RESOLVED FIXED in Firefox 11

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 577877 [details] [diff] [review]
patch v1

Trivial patch checking for gSearchEngine.
Attachment #577877 - Flags: review?(mak77)
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...
(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.
Created attachment 578565 [details] [diff] [review]
test case

Test case causing the error.
Attachment #577877 - Attachment is obsolete: true
Attachment #577877 - Flags: review?(mak77)
That's the line causing the error:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutHome.js#184
ah, so localStorage["search-engine"] is null, JSON.parse(null) is null, and we are actually doing gSearchEngine = null;
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.
Created attachment 578603 [details] [diff] [review]
patch v2

Checks if gSearchEngine is null in setupSearchEngine().
Attachment #578565 - Attachment is obsolete: true
Attachment #578603 - Flags: review?(mak77)
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
Attachment #578603 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/fx-team/rev/f40ab132ed31
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 11
https://hg.mozilla.org/mozilla-central/rev/f40ab132ed31
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.