Closed Bug 508102 Opened 15 years ago Closed 15 years ago

Warning: reference to undefined property this._pendingQuery

Categories

(Toolkit :: Places, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: neil, Assigned: sdwilsh)

References

Details

(Keywords: compat, regression)

Attachments

(1 file, 2 obsolete files)

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
Editing the summary for future Bugzilla queries.
Summary: Warning: reference to undefined property this._pendingQuery → Warning: reference to undefined property this._pendingQuery / Location bar autocomplete doesn't work anymore
(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?
Severity: major → normal
ooops...sorry!
Severity: normal → major
Assignee: nobody → sdwilsh
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #397672 - Flags: review?(dietrich)
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]
Whiteboard: [can land]
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?
(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?
(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.
(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
(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+
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #397672 - Attachment is obsolete: true
Attachment #399604 - Flags: review?(dietrich)
Whiteboard: [can land] → [needs review dietrich]
Attached patch v1.1Splinter Review
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]
I'm sheriff tomorrow, so I'll try to land it then.
Status: NEW → ASSIGNED
Whiteboard: [can land]
Status: ASSIGNED → RESOLVED
Closed: 15 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?
Comment on attachment 399607 [details] [diff] [review] v1.1 We don't need approval - it's a blocker.
Attachment #399607 - Flags: approval1.9.2?
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
> 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.
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
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?
(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
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.

Attachment

General

Created:
Updated:
Size: