Last Comment Bug 711914 - Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache
: Fetching bookmarks information during onBeforeItemRemove may break the bookma...
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: 8 Branch
: x86 Windows Vista
-- normal (vote)
: Firefox 12
Assigned To: Marco Bonardo [::mak]
: Marco Bonardo [::mak]
Depends on:
  Show dependency treegraph
Reported: 2011-12-18 23:21 PST by Usman Latif
Modified: 2013-06-01 03:02 PDT (History)
3 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

test.js (1.38 KB, text/plain)
2011-12-18 23:21 PST, Usman Latif
no flags Details
patch v1.0 (7.51 KB, patch)
2012-01-09 06:17 PST, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review

Description User image Usman Latif 2011-12-18 23:21:18 PST
Created attachment 582754 [details]

User Agent: Mozilla/5.0 (Windows NT 6.0) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.63 Safari/535.7

Steps to reproduce:

I called removeFolderChildren on a folder that contained a child folder. The child folder contained a single bookmark.

Note: This bug reports the same issue as Bug 675416. The fix for bug 675416 is not comprehensive and fails in some cases.

Actual results:

After deleting the parent folder I was able to retrieve the information of the child folder. 

It is also possible to create additional bookmarks so that one of the new bookmarks reuses the id of the deleted folder and the bookmark service returns the information of the deleted folder in place of the newly created bookmark.

Expected results:

Firefox should have thrown an exception instead.
Comment 1 User image Marco Bonardo [::mak] 2011-12-19 06:12:16 PST
thanks for filing the bug, taking to check what happens.
Comment 2 User image Marco Bonardo [::mak] 2012-01-09 06:17:58 PST
Created attachment 586981 [details] [diff] [review]
patch v1.0

The bug is caused by the calling order in the internal loop of RemoveFolderchildren. We can't change ordering in that loop since it may cause unwanted regressions. So I decided than rather than addressing each single possible failure point it's better to increase checks on the cache.
This makes the cache stronger by tracking items in the critical path, while registered they won't be added to the cache. Also asserts that each END has a corresponding BEGIN by checking the hash contents.
Comment 3 User image Marco Bonardo [::mak] 2012-01-11 02:48:33 PST
Ps: the existing assertion was catching this failure, but we didn't have a test leveraging that path. That's why I think this different approach is better, it enforces the condition.
Comment 5 User image Marco Bonardo [::mak] 2012-01-13 01:06:31 PST
Comment 6 User image Scoobidiver (away) 2013-06-01 03:02:52 PDT
*** Bug 878061 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.