498228 - Profile item doesn't look selected anymore in the profile manager

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: sreenshot

See screenshot, on that screenshot, the qq profile is selected (it is autoselected on startup), but it doesn't show up as selected anymore. You also can't make it selected again by clicking on that listitem.
This is a regression from bug 495728, backing out the patch from that bug fixes this bug.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Happens on all platforms.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch

FlushPendingNotifications doesn't do anything when called while under a script blocker. So put both calls to it outside the script blocker and be careful. Also made the presContext local var a COMPtr so we hold a reference to it and use it everywhere instead of calling PresContext() again. I'm not sure if the weak frame check I added after VerticalScroll is needed or not.

Comment 4

[Timothy Nikkel (:tnikkel)]

I haven't been able to come up with an automated test. I've been focusing on the easier-to-test symptom of items not looking selected when navigating with the arrow keys causes a scroll. I haven't even been able to come up with a listbox in a xul file that when loaded in Firefox I can reproduce the above mentioned symptom (but I can reproduce in the profile selection window on the same build). I don't know what it is that is specific to the profile selection dialog's listbox that I am missing -- I've tried mimicking as much as I can from it.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Testcase

Attached you will find a testcase which shows this behavior. It is somehow related to the focus code within the js file. I don't have time for deeper investigation. I hope it will help you.

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to comment #5)
> Created an attachment (id=383439) [details]
> Testcase
> 
> Attached you will find a testcase which shows this behavior. It is somehow
> related to the focus code within the js file. I don't have time for deeper
> investigation. I hope it will help you.

Thank you. It did help. My testcase is now working. I think something got screwed up in the build directory I was testing in, and it needs to be clobbered.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Added a reftest. For some reason the key to this bug is adding the list items by javascript instead of including them in the XUL markup.

In the process of creating the reftest I noticed another bug in this class. EnsureIndexIsVisible does not bounds check its argument against the number of items in the list box; it will happily try to ensure non-existent indexes are visible leading to bad results. It had the comment "Check to be sure we're not scrolling off the bottom of the tree", but it did not do that, so I think maybe there was some bad copy-paste from ScrollToIndex, which has the same comment but actually does that.

EnsureIndexIsVisible had another false comment. "Safe to not go off an event here, since this is coming from the box object." I'm not totally sure what it means by "coming from the box object", but I don't think it is true as EnsureIndexIsVisible gets called from javascript (I checked the stack, no box object, just javascript). But in any case we have made DoInternalPositionChangedSync safe.

Also added a script blocker to ComputeTotalRowCount: if script made changes to the content tree while we were walking it (say by removing a child after we get a local copy of the child count) we would be screwed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I'm not totally sure what it means by "coming from the box object"

That means "called directly from javascript", basically.  As in, no need to put this off on an event, because now is by definition a safe time to be running scripts.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The ComputeTotalRowCount change is definitely wrong.  While the existing code is in fact safe (because it does nothing that could trigger script), the new code with scriptblocker can run script as the function exits, which can make some of the callsites crash exploitably if |this| gets deleted.  Please please watch for this; you had similar problems with the last patch to this code, no?

This change makes EnsureIndexIsVisible O(N) where right now it's generally O(distance from where we are to index).  Are you sure that won't have adverse effects on any callers?

For the rest... I'd really like a whitespace-ignoring diff; that would make it a lot simpler to see what's really being changed.  hg's default -w behavior is broken, so what you want here is:

  [extensions]
  hgext.extdiff =
  [extdiff]
  cmd.wdiff = diff
  opts.wdiff = -p -U 8 -w -r -N

and then use hg wdiff.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3 wdiff

(In reply to comment #8)
> > I'm not totally sure what it means by "coming from the box object"
> 
> That means "called directly from javascript", basically.  As in, no need to put
> this off on an event, because now is by definition a safe time to be running
> scripts.

Can you briefly explain why it means that?

(In reply to comment #9)
> The ComputeTotalRowCount change is definitely wrong.  While the existing code
> is in fact safe (because it does nothing that could trigger script), the new
> code with scriptblocker can run script as the function exits, which can make
> some of the callsites crash exploitably if |this| gets deleted.  Please please
> watch for this; you had similar problems with the last patch to this code, no?

Ok, sorry, this was my misunderstanding. When I originally read what nsAutoScriptBlocker does, I wrongly forget the part about _synchronously_ running scripts, and only remembered the increment/decrement behaviour. So when you said that script can run when you exit a script blocker, I didn't realize you meant _synchronously_, and thus the message did not get through.

You also cleared up another misconception I had that script could be running in another thread.

> This change makes EnsureIndexIsVisible O(N) where right now it's generally
> O(distance from where we are to index).  Are you sure that won't have adverse
> effects on any callers?

We can move down the GetRowCount call to only call when needed, but I don't see a way to not call it at all while avoiding trying to scroll below the bottom. Besides, GetRowCount only does actual work if the mRowCount is not initialized, once it is initialized OnContentInserted/Removed just update the count. Any other scrolling (scripted or user) will cause it to be already initialized.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Can you briefly explain why it means that?

The only caller of nsListBoxBodyFrame::EnsureIndexIsVisible is nsListBoxObject::EnsureIndexIsVisible.  That's what it means by "coming from the box object".

The purpose of box objects is to serve as script-visible ways of accessing stuff on frames.  So any call into the box object has to be assumed to be coming from script... because it could be.

Now in this particular case all the callers into the box object are in fact coming from script, as quick mxr inspection shows.

> Besides, GetRowCount only does actual work if the mRowCount is not
> initialized,

Ah, ok.  So it's O(1) in the common case.  That's fine then.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to comment #11)
> The only caller of nsListBoxBodyFrame::EnsureIndexIsVisible is
> nsListBoxObject::EnsureIndexIsVisible.  That's what it means by "coming from
> the box object".

So nsListBoxObject doesn't show up in my stack because it is optimized out?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> So nsListBoxObject doesn't show up in my stack because it is optimized out?

No idea; is that an opt build?

Comment 14

[Boris Zbarsky [:bzbarsky]]

As far as the patch goes, you want an nsRefPtr, not an nsCOMPtr, for nsPresContext.  And I don't think you need the weakFrame check after VerticalScroll.

Fix those and put up both the -w and the whitespace-changed version?  Need the latter to push it anyway.  ;)

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: patch v4 wdiff

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: patch v4

Made requested changes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/d3f3e93bf841

Comment 18

[[e-mail]]

(In reply to comment #7)
> For some reason the key to this bug is adding the list items
> by javascript instead of including them in the XUL markup.
This is usually because when the list items are included in the XUL markup then when they are first wrapped (i.e. accessed by script) their XBL can be attached and thus they provide the .selected accessor. List items that are created by script do not acquire the .selected accessor until some other event has caused their XBL to be attached. I don't know if it is still true but at one point elements that were created by script would not get their XBL attached until they were displayed (or scrolled in to view in the case of list items).

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090628 Minefield/3.6a1pre ID:20090628031302

Comment 20

[Timothy Nikkel (:tnikkel)]

This is a regression from bug 495728, so we should consider both bugs in concert for possibly taking on branches.