bugGNUstep - Bugs: bug #28104, NSNotificationQueue may loose...

Group
 
 

bug #28104: NSNotificationQueue may loose notifications

Submitter:  Wolfgang Lux <wlux>
Submitted:  Fri 27 Nov 2009 12:35:28 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 17 Aug 2010 07:29:51 AM UTC, comment #6: 

Thanks ... I added the assertions and  applied a different fix ... putting the notifications in a the array before removing the registrations from the list and posting the notifications.  This avoids the re-entrancy problem because all the list items are removed before notify() can be called again.

Richard Frith-Macdonald <CaS>
Group Member
Mon 16 Aug 2010 08:10:17 AM UTC, comment #5: 

Almost nine month later it turns out that the fix has introduced a new problem: The modified notify() function is no longer reentrant, i.e., programs will crash if one of the notifiees recursively enters the event loop and removes (and thus deallocates) some of the cached items from the queue. The (first) attached patch fixes this problem by taking just one element from the queue and repeating this process until no notification with a matching mode remains. This leads to an O(n^2) complexity, but at least for the gui programs I've tested this seems acceptable since the asap and idle queues contain only few if any elements. However, I'm sure you'll come up with a more efficient implementation if you think the quadratic complexity is unacceptable.

PS: During debugging I've stumbled over two redundant conditional assignments in remove_from_queue_no_release. The second patch removes them and instead introduces two assertions to verify the integrity of the queue when removing the head and tail elements. These would have turned out to be a bit more helpful at hinting at the bug than a simple crash.

(file #21230, file #21231)

Wolfgang Lux <wlux>
Fri 27 Nov 2009 04:13:45 PM UTC, comment #4: 

Seems to be fixed.

Richard Frith-Macdonald <CaS>
Group Member
Fri 27 Nov 2009 01:45:44 PM UTC, comment #3: 

I gave it a try and it works fine. The user defaults are now always updated after moving a menu.

Thanks for fixing the issue.

Wolfgang Lux <wlux>
Fri 27 Nov 2009 10:43:40 AM UTC, comment #2: 

I've quickly rewritten the posting code ... please see how it works.

Richard Frith-Macdonald <CaS>
Group Member
Fri 27 Nov 2009 09:25:25 AM UTC, comment #1: 

Very well spotted ... that's pretty obscure and must have been difficult to track down.
It certainly makes sense to only remove notifications if the loop is known to be running in a matching mode.

Richard Frith-Macdonald <CaS>
Group Member
Fri 27 Nov 2009 12:35:28 AM UTC, original submission:  

Despite my recent fix to re-enable saving of menu positions in the user defaults, I sometimes observe that positions are not saved. I've tracked down the problem to the point that NSMenu enqueues a special notification on the default NSNotificationQueue. The enqueued notification is supposed to be posted with notification style NSPostASAP in NSDefaultRunLoop mode, i.e.,  after mouse tracking has ended and the menu has been put to its new position.

By setting appropriate breakpoints I could verify that GSPrivateNotifyASAP and notifyASAP indeed are called. The problem is that notifyASAP (and ditto notifyIdle) just takes every notification off the respective queue and passes it on to _postNotification:forModes:. This method in turn posts the notification if the run loop is in the correct mode and discards it otherwise, which is obviously wrong. Either notifyASAP and notifyIdle should only take notifications with a suitable run loop mode off their queues or _postNotification:forModes: should be replaced by a method that pushes those notifications which cannot be posted back onto the appropriate queue.

Wolfgang Lux <wlux>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #21230:  NSNotificationQueue-fix.m added by wlux (2KiB - application/octet-stream)
file #21231:  NSNotificationQueue-patch.m added by wlux (664B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by wlux (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-08-16 wlux Attached File- Added NSNotificationQueue-fix.m, #21230
        Attached File- Added NSNotificationQueue-patch.m, #21231
    2009-11-27 CaS StatusIn Progress Fixed
        Open/ClosedIn Test Closed
    2009-11-27 CaS Open/ClosedOpen In Test
    2009-11-27 CaS StatusNone In Progress

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code