515463 - new async autocomplete does not always respect behavior pref changes

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Marco Bonardo [:mak]]

If you look at mixed (history and bookmarks) results and you flip the UI pref through values values, you can end up viewing bookmarks when history is selected, or viceversa (i've also seen results with Nothing selected)

Comment 1

[Ed Lee :Mardak]

Was working in Firefox 3.5.. might be related to the rewrite. I do see that we register a "" pref observer and reload all the prefs.

Comment 2

[Shawn Wilsher :sdwilsh]

We only register for "browser.urlbar." changes.

Comment 3

[Dietrich Ayala (:dietrich)]

blocking for broken UI and potential privacy exposure.

Comment 4

[Johnathan Nightingale [:johnath]]

Not healthy to have blockers assigned to nobody@ - shawn, mardak, who's taking this?

Comment 5

[Dietrich Ayala (:dietrich)]

assigning to shawn since it sounds like an asyncbar regression. reassign as needed.

Comment 6

[Shawn Wilsher :sdwilsh]

Can somebody give me more specific steps to reproduce?  I thought I understood this bug, but it appears that I do not!

Comment 7

[Marco Bonardo [:mak]]

1. create a new profile
2. search for "moz" in the locationbar, you get results
3. change options -> privacy -> locationbar to "nothing"
4. search for "moz" in the locationbar, you don't get results
5. change options -> privacy -> locationbar to anything other than "nothing"
6. search for "moz" in the locationbar, you don't get results!

iirc i was able to reproduce even in opposite directions, so not willing to see results i was seeing them.
For example actually i have the pref set to "bookmarks" but i only see history results...

restarting browser fixes the issue.

Comment 8

[Shawn Wilsher :sdwilsh]

Silly me (and reviewers!).  We don't hold onto our preference branch, so our observer can disappear.  D'oh!  Patch RSN!

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 10

[Ed Lee :Mardak]

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

>+      let pb = this._prefs.QueryInterface(Ci.nsIPrefBranch2);
>       pb.addObserver("", this, false);
Why do we need to hold a reference to the prefs branch? Is that just so that there's a cycle between autocomplete and prefs and the GC doesn't collect it? I'm still not sure what's being collected to begin with..

Also, isn't that last argument "false" supposed to keep a strong reference -- not that autocomplete is getting collected in the first place.. ?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Why do we need to hold a reference to the prefs branch? Is that just so that
> there's a cycle between autocomplete and prefs and the GC doesn't collect it?
> I'm still not sure what's being collected to begin with..
Pretty much.  The preference branch will get a refcount of zero, and it will be deleted, taking our observer with it.

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

per comments on irc

Comment 13

[Ed Lee :Mardak]

Ah, that makes much more sense. I was getting confused trying to figure out how the pref service was going away, but it's just the particular branch we're requesting that disappears.

Comment 14

[Marco Bonardo [:mak]]

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

reviewed on IRC and manually tested working, r=me

Still this gc thing is pretty strange looking, i would have expected that being the pref service still alive, branches derived from it were alive as well.
I think could make sense to file a bug that blocks this and fuel issue about this thing, if no bug exists as of today. Could even be correct to avoid holding useless branches in memory but it's quite error-prone.

Comment 15

[Marco Bonardo [:mak]]

i was thinking if flagging in-litmus is correct. the bug is reproduceable, but having a bug depending on gc on litmus could bring random results... maybe we should just flag it for generic: "set pref and check, unset pref and check"

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> i was thinking if flagging in-litmus is correct. the bug is reproduceable, but
> having a bug depending on gc on litmus could bring random results... maybe we
> should just flag it for generic: "set pref and check, unset pref and check"
Agreed.

(In reply to comment #14)
> Still this gc thing is pretty strange looking, i would have expected that being
> the pref service still alive, branches derived from it were alive as well.
> I think could make sense to file a bug that blocks this and fuel issue about
> this thing, if no bug exists as of today. Could even be correct to avoid
> holding useless branches in memory but it's quite error-prone.
I recall discussions about this in the past saying that this is an invariant of the pref observer API.

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/62d4f947aab5

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/66263046d823

Comment 19

[Tracy Walker [:tracy]]

This is covered in litmus FFT set.

Comment 20

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20091111 Minefield/3.7a1pre