508102 - Warning: reference to undefined property this._pendingQuery

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[[e-mail]]

Both XPFE and Toolkit autocomplete implementations call stopSearch even before any searches have been done. Unfortunately nsPlacesAutoComplete.js wasn't expecting this, and generates a warning and an exception, which Toolkit happens to ignore as it's written in C++ but XPFE's JS autocomplete stops working.

Comment 2

[Jason Bassford]

Editing the summary for future Bugzilla queries.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> *** Bug 508055 has been marked as a duplicate of this bug. ***

This confirms the timeframe reported in that bug:
changeset b2fd524aff54 of bug 455555, merged to m-c "by" changeset 8cff4bd2121a.

Comment 4

[Marco Perez [:mpk]]

ooops...sorry!

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 397672 [details] [diff] [review]
v1.0

r=me, thanks

Comment 9

[[e-mail]]

Comment on attachment 397672 [details] [diff] [review]
v1.0

>+    catch (e) {
This stops the exception but not the warning :-(

Comment 10

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

Shouldn't you just null check _pendingQuery in _stopActiveQuery anyways, rather than wrapping the entire function call in a try/catch?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Shouldn't you just null check _pendingQuery in _stopActiveQuery anyways, rather
> than wrapping the entire function call in a try/catch?
No, because everywhere else we call it _pendingQuery should be defined (and it'd be a logic error if it wasn't).  I want it to be an error in all cases except for this one to ensure our state is correct.

(In reply to comment #9)
> This stops the exception but not the warning :-(
On the command line?  We should maybe fix that bug then, but that's not a compelling reason to not use try-catch.

Comment 12

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

(In reply to comment #11)
> No, because everywhere else we call it _pendingQuery should be defined (and
> it'd be a logic error if it wasn't).  I want it to be an error in all cases
> except for this one to ensure our state is correct.

Why not just check _pendingQuery in stopQuery, then?

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Why not just check _pendingQuery in stopQuery, then?
The original goal of pulling out the logic in _stopActiveQuery was to simplify the call sites to not have to worry about various variables.  This patch doesn't care what the state is - it unconditionally drops the error if it happens.

Comment 14

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

(In reply to comment #13)
> (In reply to comment #12)
> > Why not just check _pendingQuery in stopQuery, then?
> The original goal of pulling out the logic in _stopActiveQuery was to simplify
> the call sites to not have to worry about various variables.

And yet you just finished telling me how that call site was exceptional :)

Let's not use try/catch to blanket over all exceptions when there's an easy way to test exactly what we need to test here.

Comment 15

[[e-mail]]

(In reply to comment #11)
> (In reply to comment #9)
> > This stops the exception but not the warning :-(
> On the command line?
No, in the Error Console, assuming you turn all the relevant prefs on:

Warning: reference to undefined property this._pendingQuery
Source File: components/nsPlacesAutoComplete.js
Line: 635

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #12)
> (In reply to comment #11)
> > No, because everywhere else we call it _pendingQuery should be defined (and
> > it'd be a logic error if it wasn't).  I want it to be an error in all cases
> > except for this one to ensure our state is correct.
> 
> Why not just check _pendingQuery in stopQuery, then?

this sounds better. the comment in the patch even references a private member not used anywhere in that chunk of code, which highlights the problem gavin's pointing out.

also, should fix the console spam.

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 20

[Dietrich Ayala (:dietrich)]

Comment on attachment 399607 [details] [diff] [review]
v1.1

r=me, thanks

Comment 21

[Shawn Wilsher :sdwilsh]

I'm sheriff tomorrow, so I'll try to land it then.

Comment 22

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/a1d8b365bc51

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 399607 [details] [diff] [review]
v1.1

asking approval, small impact change with a test.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 399607 [details] [diff] [review]
v1.1

We don't need approval - it's a blocker.

Comment 25

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/46fde5c74fa6

Comment 26

[Alexander Kluss]

Doesn't work with Seamonkey "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090912 SeaMonkey/2.1a1pre"
With which build is the fix at the trunk?

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> *** Bug 508055 has been marked as a duplicate of this bug. ***

The fix here may have helped SeaMonkey, though it's not obvious.
I reopened that bug.

Comment 28

[Jason Bassford]

> Doesn't work with Seamonkey "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
> rv:1.9.3a1pre) Gecko/20090912 SeaMonkey/2.1a1pre"
> With which build is the fix at the trunk?

Confirmed.  It looks like this either did not get checked into the SeaMonkey trunk builds (it also fails with a 9/13 build off of comm-central) - or the fix simply doesn't work with the trunk. 

With a 9/13 comm-1.9.1 everything is fine now, so the branch is okay.

Comment 29

[Oliver Schoett]

Autocomplete still does not work in comm-central-trunk build

   Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090921 SeaMonkey/2.1a1pre

so this appears to be not fixed yet

Comment 30

[Johannes Leushuis]

Can someone change the status of this bug from resolved fixed to unresolved? Autocomplete indeed still does not work in the location bar in Seamonkey2.1a1pre Build ID:{20090924013522} And the discussion of this bug goes parallel with the discussion of bug 508055 [SeaMonkey 2.1] Location bar autocomplete doesn't work anymore. Is this how it should be?

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #30)
> Can someone change the status of this bug from resolved fixed to unresolved?
> Autocomplete indeed still does not work in the location bar in
> Seamonkey2.1a1pre Build ID:{20090924013522} And the discussion of this bug goes
> parallel with the discussion of bug 508055 [SeaMonkey 2.1] Location bar
> autocomplete doesn't work anymore. Is this how it should be?
I'm not fixing SeaMonkey code.  I've fixed the toolkit issue, and this bug is resolved.  Bug 508055 is tracking their issue.

Comment 32

[Jason Bassford]

This bug should remain closed.  Any further work on the trunk should be addressed by bug 508055 - which has been reopened.  We only need one bug tracking the problem, and it shouldn't be this one any more.  (It does appear as if the problem on now the trunk is something different than what this bug fixed.)