Warning: reference to undefined property this._pendingQuery

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Toolkit
Places
--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: sdwilsh)

Tracking

({compat, regression})

Trunk
mozilla1.9.3a1
compat, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Keywords: compat
Version: unspecified → Trunk
(Assignee)

Updated

8 years ago
Duplicate of this bug: 508055

Comment 2

8 years ago
Editing the summary for future Bugzilla queries.

Updated

8 years ago
Summary: Warning: reference to undefined property this._pendingQuery → Warning: reference to undefined property this._pendingQuery / Location bar autocomplete doesn't work anymore
Blocks: 470184
(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.
Severity: normal → major
Flags: blocking1.9.2?

Updated

8 years ago
Severity: major → normal

Comment 4

8 years ago
ooops...sorry!
Severity: normal → major
(Assignee)

Updated

8 years ago
Duplicate of this bug: 510589

Updated

8 years ago
Duplicate of this bug: 510196
Assignee: nobody → sdwilsh
(Assignee)

Comment 7

8 years ago
Created attachment 397672 [details] [diff] [review]
v1.0
Attachment #397672 - Flags: review?(dietrich)
(Assignee)

Updated

8 years ago
Whiteboard: [needs review dietrich]
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 397672 [details] [diff] [review]
v1.0

r=me, thanks
Attachment #397672 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich]
(Assignee)

Updated

8 years ago
Whiteboard: [can land]
(Reporter)

Comment 9

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

>+    catch (e) {
This stops the exception but not the warning :-(
Shouldn't you just null check _pendingQuery in _stopActiveQuery anyways, rather than wrapping the entire function call in a try/catch?
(Assignee)

Comment 11

8 years ago
(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.
(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?
(Assignee)

Comment 13

8 years ago
(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.
(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.
(Reporter)

Comment 15

8 years ago
(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

Updated

8 years ago
Duplicate of this bug: 514133
(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.
Flags: blocking1.9.2? → blocking1.9.2+
(Assignee)

Comment 18

8 years ago
Created attachment 399604 [details] [diff] [review]
v1.1
Attachment #397672 - Attachment is obsolete: true
Attachment #399604 - Flags: review?(dietrich)
(Assignee)

Updated

8 years ago
Whiteboard: [can land] → [needs review dietrich]
(Assignee)

Comment 19

8 years ago
Created attachment 399607 [details] [diff] [review]
v1.1
Attachment #399604 - Attachment is obsolete: true
Attachment #399607 - Flags: review?(dietrich)
Attachment #399604 - Flags: review?(dietrich)
Comment on attachment 399607 [details] [diff] [review]
v1.1

r=me, thanks
Attachment #399607 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich]
(Assignee)

Comment 21

8 years ago
I'm sheriff tomorrow, so I'll try to land it then.
Status: NEW → ASSIGNED
Whiteboard: [can land]
(Assignee)

Comment 22

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a1d8b365bc51
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Comment on attachment 399607 [details] [diff] [review]
v1.1

asking approval, small impact change with a test.
Attachment #399607 - Flags: approval1.9.2?
(Assignee)

Comment 24

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

We don't need approval - it's a blocker.
Attachment #399607 - Flags: approval1.9.2?
(Assignee)

Comment 25

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/46fde5c74fa6
status1.9.2: --- → beta1-fixed

Comment 26

8 years ago
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?
Depends on: 508055
(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.
Blocks: 508055
No longer depends on: 508055

Comment 28

8 years ago
> 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

8 years ago
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

8 years ago
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?
(Assignee)

Comment 31

8 years ago
(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.
Summary: Warning: reference to undefined property this._pendingQuery / Location bar autocomplete doesn't work anymore → Warning: reference to undefined property this._pendingQuery

Comment 32

8 years ago
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.)
No longer blocks: 508055
You need to log in before you can comment on or make changes to this bug.