[Author: avi]

Allow adding a new sheet in a sheet's closing callback in GTMWindowSheetController. GTMWindowSheetController does not completely remove a sheet until after the callback has been invoked. This causes an assert to trigger if another sheet is launched in the callback, which is inconsistent with normal Cocoa sheets. Switch the order of the two operations and add a unit test. This affects http://crbug.com/56948 . Patch by davidben@chromium.org .

R=dmaclach
DELTA=112  (106 added, 5 deleted, 1 changed)

2 files changed, 107 insertions, 6 deletions

diff --git a/AppKit/GTMWindowSheetController.m b/AppKit/GTMWindowSheetController.m
index f7c87a3..60cd611 100644
--- a/AppKit/GTMWindowSheetController.m
+++ b/AppKit/GTMWindowSheetController.m
@@ -523,7 +523,10 @@ willPositionSheet:(NSWindow*)sheet
         contextInfo:(void*)contextInfo
            arg1Size:(int)size {
   NSValue* viewKey = [NSValue valueWithNonretainedObject:(NSView*)contextInfo];
-  GTMWSCSheetInfo* sheetInfo = [sheets_ objectForKey:viewKey];
+  // Retain a reference to sheetInfo so we can use it after it is
+  // removed from sheets_.
+  GTMWSCSheetInfo* sheetInfo =
+      [[[sheets_ objectForKey:viewKey] retain] autorelease];
   _GTMDevAssert(sheetInfo, @"Could not find information about the sheet that "
                            @"just ended");
   _GTMDevAssert(size == 8 || size == 32 || size == 64,
@@ -537,6 +540,12 @@ willPositionSheet:(NSWindow*)sheet
                 name:NSViewFrameDidChangeNotification
               object:contextInfo];
 
+  // We clean up the sheet before calling the callback so that the
+  // callback is free to fire another sheet if it so desires.
+  [window_ removeChildWindow:sheetInfo->overlayWindow_];
+  [sheetInfo->overlayWindow_ release];
+  [sheets_ removeObjectForKey:viewKey];
+
   NSInvocation* invocation =
       [NSInvocation invocationWithMethodSignature:
        [sheetInfo->modalDelegate_
@@ -555,11 +564,6 @@ willPositionSheet:(NSWindow*)sheet
   }
   [invocation setArgument:&sheetInfo->contextInfo_ atIndex:4];
   [invocation invokeWithTarget:sheetInfo->modalDelegate_];
-
-  [window_ removeChildWindow:sheetInfo->overlayWindow_];
-  [sheetInfo->overlayWindow_ release];
-
-  [sheets_ removeObjectForKey:viewKey];
 }
 
 - (void)systemRequestsVisibilityForWindow:(NSWindow*)window {
diff --git a/AppKit/GTMWindowSheetControllerTest.m b/AppKit/GTMWindowSheetControllerTest.m
index b2cb7b2..65ef084 100644
--- a/AppKit/GTMWindowSheetControllerTest.m
+++ b/AppKit/GTMWindowSheetControllerTest.m
@@ -25,12 +25,16 @@
                                            NSTabViewDelegate> {
  @private
   GTMWindowSheetController *sheetController_;
+  NSWindow* window_;
   BOOL didAlertClose_;
   BOOL didSheetClose_;
 }
 - (void)alertDidEnd:(NSAlert *)alert
          returnCode:(NSInteger)returnCode
             context:(void *)context;
+- (void)openSecondSheet:(NSAlert *)alert
+             returnCode:(NSInteger)returnCode
+                context:(void *)context;
 - (void)sheetDidEnd:(NSWindow *)sheet
          returnCode:(NSInteger)returnCode
             context:(void *)context;
@@ -172,12 +176,105 @@
   STAssertTrue(didSheetClose_, @"Sheet should have closed");
 }
 
+- (void)testOpenSheetAfterFirst {
+  // Set up window
+  window_ =
+    [[[NSWindow alloc] initWithContentRect:NSMakeRect(100, 100, 600, 600)
+                                 styleMask:NSTitledWindowMask
+                                   backing:NSBackingStoreBuffered
+                                     defer:NO] autorelease];
+  STAssertNotNil(window_, @"Could not allocate window");
+
+  sheetController_ =
+      [[[GTMWindowSheetController alloc] initWithWindow:window_
+                                               delegate:self] autorelease];
+
+  STAssertFalse([sheetController_ isSheetAttachedToView:
+                 [window_ contentView]],
+                @"Sheet should not be attached to current view");
+  STAssertEquals([[sheetController_ viewsWithAttachedSheets] count],
+                 (NSUInteger)0,
+                 @"Should have no views with sheets");
+
+  // Pop alert on first tab
+  NSAlert* alert = [[NSAlert alloc] init];
+
+  [alert setMessageText:@"Hell Has Broken Loose."];
+  [alert setInformativeText:@"All hell has broken loose. You may want to run "
+                            @"outside screaming and waving your arms around "
+                            @"wildly."];
+
+  NSButton *alertButton = [alert addButtonWithTitle:@"OK"];
+
+  // Allocate a second sheet to be launched by the alert
+  NSPanel *sheet =
+      [[[NSPanel alloc] initWithContentRect:NSMakeRect(0, 0, 300, 200)
+                                  styleMask:NSTitledWindowMask
+                                    backing:NSBackingStoreBuffered
+                                      defer:NO] autorelease];
+
+  [sheetController_ beginSystemSheet:alert
+                        modalForView:[window_ contentView]
+                      withParameters:[NSArray arrayWithObjects:
+                                      [NSNull null],
+                                      self,
+                                      [NSValue valueWithPointer:
+                                        @selector(openSecondSheet:returnCode:context:)],
+                                      [NSValue valueWithPointer:sheet],
+                                      nil]];
+  didAlertClose_ = NO;
+  didSheetClose_ = NO;
+
+  STAssertTrue([sheetController_ isSheetAttachedToView:
+                [window_ contentView]],
+                @"Sheet should be attached to view");
+  STAssertEquals([[sheetController_ viewsWithAttachedSheets] count],
+                 (NSUInteger)1,
+                 @"Should have one view with sheets");
+
+  // Close alert
+  [alertButton performClick:self];
+
+  STAssertTrue([sheetController_ isSheetAttachedToView:
+                [window_ contentView]],
+                @"Second sheet should be attached to view");
+  STAssertEquals([[sheetController_ viewsWithAttachedSheets] count],
+                 (NSUInteger)1,
+                 @"Should have one view with sheets");
+  STAssertTrue(didAlertClose_, @"Alert should have closed");
+
+  // Close sheet
+  [[NSApplication sharedApplication] endSheet:sheet returnCode:NSOKButton];
+
+  STAssertFalse([sheetController_ isSheetAttachedToView:
+                 [window_ contentView]],
+                @"Sheet should not be attached to current view");
+  STAssertEquals([[sheetController_ viewsWithAttachedSheets] count],
+                 (NSUInteger)0,
+                 @"Should have no views with sheets");
+  STAssertTrue(didSheetClose_, @"Sheet should have closed");
+}
+
 - (void)alertDidEnd:(NSAlert *)alert
          returnCode:(NSInteger)returnCode
             context:(void *)context {
   didAlertClose_ = YES;
 }
 
+- (void)openSecondSheet:(NSAlert *)alert
+             returnCode:(NSInteger)returnCode
+                context:(void *)context {
+  didAlertClose_ = YES;
+
+  NSPanel* sheet = context;
+  // Pop a second sheet
+  [sheetController_ beginSheet:sheet
+                  modalForView:[window_ contentView]
+                 modalDelegate:self
+                didEndSelector:@selector(sheetDidEnd:returnCode:context:)
+                   contextInfo:nil];
+}
+
 - (void)sheetDidEnd:(NSWindow *)sheet
          returnCode:(NSInteger)returnCode
             context:(void *)context {