620198 - Right pane tree of Library is not updated when i delete a selected folder in left pane tree

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101217
Firefox/4.0b9pre ID:20101217030324

Right pane tree does not update when i delete a selected folder in left pane tree

Reproducible: Always

Steps to Reproduce:
1. Start Minefield
2. Open Library
3. Select a folder in left pane tree
4. push DEL key

Actual Results:
 Right pane tree does not update, 
 Bookmarks which were in deleted folder are shown in right pane tree.
 I can not scroll right pane by mouse whell and by drag thumb.
 I can not select bookmarks in right pane tree by mouse click.
 When I click a bookmarks in right pane tree, wrong data(name, url etc) are shown in detail pene.
 When select other folder in left pane, right pane tree does not update.

Expected Results:
 Library should work properly

Comment 1

[Alice0775 White]

This does not happens on Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.14pre) Gecko/20101215 Firefox/3.6.14pre ID:20101215033249

Comment 2

[Alice0775 White]

Regressio window:
Works:
http://hg.mozilla.org/mozilla-central/rev/4d99a78ddae4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101111 Firefox/4.0b8pre ID:20101112131930
Fails:
http://hg.mozilla.org/mozilla-central/rev/1911a8629c39
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101112 Firefox/4.0b8pre ID:20101112143830
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4d99a78ddae4&tochange=1911a8629c39

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

happens also on XP 32bit and on 7 64bit with 64bit Fx4

Comment 4

[Alice0775 White]

screen capture
It happens on new profile.
http://www.youtube.com/watch?v=3UUPf0m4sUY

Comment 5

[Mehdi Mulani [:mmm] (I don't check this)]

Also seeing this on Mac and looking into it.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I'm guessing status2.0? wasn't meant to be set but blocking2.0? was.

Comment 8

[Dietrich Ayala (:dietrich)]

Blocking+ on this regression. We can't trade perf for brokenness.

Comment 9

[Mehdi Mulani [:mmm] (I don't check this)]

Noticing some weird behaviour with this.
After deleting a bookmarks folder with more than 5 items from the left pane (more than 5 so it causes a transaction batch), selecting other bookmarks folders in the left pane will not cause the right side UI to update.

However, when the Library loses focus the right side UI will update.

I've been trying to track it down in the front end UI but haven't made much progress. I think some UI thinks a batching is still going on but haven't able to confirm this. Also seems a bit similar to bug 617956.

Comment 10

[Mehdi Mulani [:mmm] (I don't check this)]

It seems that the result observer for the right hand side is not being notified during OnEndUpdateBatch. My first guess was to hook up nsNavHistory::StopObserving to nsNavHistory::OnEndUpdateBatch but this did not help.

Not sure how to proceed.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thanks, I'll take a look if I can figure it out, if you want to work on other blockers feel free to reassign to me.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Stealing since this is a bit more complicated than I expected, I have a fix but it's not exactly what I was expecting.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

I don't like the fix so much, but rethinking to it, it's the simplest and less regression-prone thing I can do without touching APIs. This also fixes Bug 394353 since I was testing this bug in the library with tags. Both fixes have tests.

The problem is that the right pane gets replaced when the left pane ends its batch, the replacement tree end up being out of sync due to the tree batch. Fixing it in result code would mixup things too much at this stage.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 503578 [details] [diff] [review]
patch v1.0

>-  onItemAdded: function EIO_onItemAdded(aItemId, aFolder, aIndex, aItemType,
>+  onItemAdded: function EIO_onItemAdded(aItemId, aParentId, aIndex, aItemType,

how is this chunk of the patch related to this bug?

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #14)
> Comment on attachment 503578 [details] [diff] [review]
> patch v1.0
> 
> >-  onItemAdded: function EIO_onItemAdded(aItemId, aFolder, aIndex, aItemType,
> >+  onItemAdded: function EIO_onItemAdded(aItemId, aParentId, aIndex, aItemType,
> 
> how is this chunk of the patch related to this bug?

it's not, as I wrote in comment 13 I was testing tags to check for this bug fix, and I ended up fixing Bug 394353. I can split them, I did not do at first since the fix was pretty small, but it's easy to do.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

splitted patch

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 504443 [details] [diff] [review]
patch v1.1

looks fine, thanks! r=me

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/projects/places/rev/9c6b3e840bf2

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

follow-up for a permaorange in the test, a previous test was not removing its bookmark, after checking the cleanup worked, I also made the uri unique to better identify it.
http://hg.mozilla.org/projects/places/rev/215402e72c24

Comment 20

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/9c6b3e840bf2
http://hg.mozilla.org/mozilla-central/rev/215402e72c24