Closed
Bug 508102
Opened 15 years ago
Closed 15 years ago
Warning: reference to undefined property this._pendingQuery
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
3.98 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
Editing the summary for future Bugzilla queries.
Updated•15 years ago
|
Summary: Warning: reference to undefined property this._pendingQuery → Warning: reference to undefined property this._pendingQuery / Location bar autocomplete doesn't work anymore
Updated•15 years ago
|
Blocks: CcMcBuildIssues
Comment 3•15 years ago
|
||
(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•15 years ago
|
Severity: major → normal
Updated•15 years ago
|
Assignee: nobody → sdwilsh
Assignee | ||
Comment 7•15 years ago
|
||
Attachment #397672 -
Flags: review?(dietrich)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review dietrich]
Target Milestone: --- → mozilla1.9.3a1
Comment 8•15 years ago
|
||
Comment on attachment 397672 [details] [diff] [review]
v1.0
r=me, thanks
Attachment #397672 -
Flags: review?(dietrich) → review+
Updated•15 years ago
|
Whiteboard: [needs review dietrich]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [can land]
Reporter | ||
Comment 9•15 years ago
|
||
Comment on attachment 397672 [details] [diff] [review]
v1.0
>+ catch (e) {
This stops the exception but not the warning :-(
Comment 10•15 years ago
|
||
Shouldn't you just null check _pendingQuery in _stopActiveQuery anyways, rather than wrapping the entire function call in a try/catch?
Assignee | ||
Comment 11•15 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.
Comment 12•15 years ago
|
||
(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•15 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.
Comment 14•15 years ago
|
||
(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•15 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
Comment 17•15 years ago
|
||
(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.
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Assignee | ||
Comment 18•15 years ago
|
||
Attachment #397672 -
Attachment is obsolete: true
Attachment #399604 -
Flags: review?(dietrich)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [can land] → [needs review dietrich]
Assignee | ||
Comment 19•15 years ago
|
||
Attachment #399604 -
Attachment is obsolete: true
Attachment #399607 -
Flags: review?(dietrich)
Attachment #399604 -
Flags: review?(dietrich)
Comment 20•15 years ago
|
||
Comment on attachment 399607 [details] [diff] [review]
v1.1
r=me, thanks
Attachment #399607 -
Flags: review?(dietrich) → review+
Updated•15 years ago
|
Whiteboard: [needs review dietrich]
Assignee | ||
Comment 21•15 years ago
|
||
I'm sheriff tomorrow, so I'll try to land it then.
Status: NEW → ASSIGNED
Whiteboard: [can land]
Assignee | ||
Comment 22•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Comment 23•15 years ago
|
||
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•15 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•15 years ago
|
||
status1.9.2:
--- → beta1-fixed
Comment 26•15 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?
Comment 27•15 years ago
|
||
(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.
Updated•15 years ago
|
Comment 28•15 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•15 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•15 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•15 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•15 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.)
You need to log in
before you can comment on or make changes to this bug.
Description
•