Closed Bug 529820 Opened 10 years ago Closed 10 years ago

nsCategoryCache is slightly unsafe

Categories

(Core :: XPCOM, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: timeless, Assigned: timeless)

Details

Attachments

(1 file, 2 obsolete files)

today nsCategoryObserver::nsCategoryObserver does the following:

obs1. get a complete list of things from nsCategoryManager
obs2. slowly loop through its list and make a call to an observer
other1. can trigger a change to nsCategoryManager
manager1. can mutate the category the observer enumerated
manager2. will fire an async event
other2. an observer can push an event queue
obs0. will miss the async event from manager2 because it isn't listening
obs2. will continue to slowly loop through its list until it finishes
obs3. will register observers (too late for obs0)

the first response people have is "why not listen early?"

obs1. get a complete list of things from nsCategoryManager
obs2. slowly loop through its list and make a call to an observer (initial_a)
other1. triggers a change to nsCategoryManager (replace "b")
manager1. can mutate the category the observer enumerated
manager2. fires async event mutated_remove_b
manager2. fires async event mutated_add_b 
other2. an observer can push an event queue
other3. receives mutated_remove_b (before initial_b)
other4. receives mutated_add_b (before initial_b)
obs2. slowly loop through its list and make a call to an observer (initial_b)
other1. gets inital_b after mutated_add_b and is thoroughly confused
obs2. will continue to slowly loop through its list until it finishes
Attached patch patch (obsolete) — Splinter Review
this ensures we don't *miss* events, it doesn't promise an absolutely strict order, but i think it's still a better improvement than the other approach
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #413339 - Flags: review?(cbiesinger)
Comment on attachment 413339 [details] [diff] [review]
patch

+  for (PRUint32 i = entries.Length(); i-- > 0; )

this is horribly unreadable. if you want to count down, use a signed integer and write the loop like:

  for (PRInt32 i = entries.Length() - 1; i >= 0; --i)

or just count up.
Attachment #413339 - Flags: review?(cbiesinger) → review-
Attached patch like this? (obsolete) — Splinter Review
Attachment #413339 - Attachment is obsolete: true
Attachment #430890 - Flags: review?(cbiesinger)
Attachment #430890 - Flags: review?(cbiesinger) → review+
Attached patch like thisSplinter Review
Attachment #430890 - Attachment is obsolete: true
Attachment #439233 - Flags: review+
tryserver thinks like this would be fine
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/c12bfe27e805
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.