Last Comment Bug 706398 - "gSearchEngine is null" when about:home is closed early
: "gSearchEngine is null" when about:home is closed early
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-30 00:02 PST by Tim Taubert [:ttaubert]
Modified: 2011-12-06 00:01 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (742 bytes, patch)
2011-11-30 00:05 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
test case (1.71 KB, patch)
2011-12-02 06:28 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (1.81 KB, patch)
2011-12-02 08:40 PST, Tim Taubert [:ttaubert]
mak77: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-11-30 00:02:43 PST
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] 2011-11-30 00:05:00 PST
Created attachment 577877 [details] [diff] [review]
patch v1

Trivial patch checking for gSearchEngine.
Comment 2 Marco Bonardo [::mak] 2011-12-01 08:57:21 PST
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] 2011-12-02 06:18:58 PST
(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] 2011-12-02 06:28:40 PST
Created attachment 578565 [details] [diff] [review]
test case

Test case causing the error.
Comment 5 Tim Taubert [:ttaubert] 2011-12-02 06:39:06 PST
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] 2011-12-02 07:17:05 PST
ah, so localStorage["search-engine"] is null, JSON.parse(null) is null, and we are actually doing gSearchEngine = null;
Comment 7 Marco Bonardo [::mak] 2011-12-02 07:22:08 PST
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] 2011-12-02 08:40:13 PST
Created attachment 578603 [details] [diff] [review]
patch v2

Checks if gSearchEngine is null in setupSearchEngine().
Comment 9 Marco Bonardo [::mak] 2011-12-02 08:54:02 PST
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] 2011-12-02 09:15:36 PST
https://hg.mozilla.org/integration/fx-team/rev/f40ab132ed31
Comment 11 Tim Taubert [:ttaubert] 2011-12-06 00:01:58 PST
https://hg.mozilla.org/mozilla-central/rev/f40ab132ed31

Note You need to log in before you can comment on or make changes to this bug.