Last Comment Bug 678607 - [10.7] crash with two finger swipe
: [10.7] crash with two finger swipe
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla9
Assigned To: Steven Michaud [:smichaud] (Retired)
:
: Markus Stange [:mstange]
Mentors:
: 673456 679438 681781 (view as bug list)
Depends on: 682445
Blocks: lion-compatibility 668953
  Show dependency treegraph
 
Reported: 2011-08-12 13:41 PDT by Asa Dotzler [:asa]
Modified: 2011-08-29 23:31 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
retain anEvent (3.33 KB, patch)
2011-08-16 08:11 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
Fix (9.51 KB, patch)
2011-08-17 09:05 PDT, Steven Michaud [:smichaud] (Retired)
b56girard: review+
Details | Diff | Splinter Review
il produced by clang (3.30 KB, text/plain)
2011-08-17 11:10 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details
another small testcase (5.00 KB, text/plain)
2011-08-17 13:59 PDT, Markus Stange [:mstange]
no flags Details
another small testcase (5.13 KB, text/plain)
2011-08-17 14:15 PDT, Markus Stange [:mstange]
no flags Details
Fix rev1 (remove unneeded code) (3.49 KB, patch)
2011-08-18 13:09 PDT, Steven Michaud [:smichaud] (Retired)
b56girard: review+
Details | Diff | Splinter Review
Markus' 2nd testcase with small change (7.62 KB, text/plain)
2011-08-19 15:31 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details
IL produce by llvm-gcc (the "gcc" in xcode 4.1) for sChildView.mm (187.80 KB, application/x-bzip2)
2011-08-22 15:38 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details

Description Asa Dotzler [:asa] 2011-08-12 13:41:48 PDT
With the new two-finger swipe (implemented in Bug 668953 bug 668953) for back and forward navigation on Mac OS X Lion, I can, with some reproducibility, cause Firefox to crash. 

Steps to reproduce: 

1. visit five or six addresses in the same tab.
2. swipe swipe swipe to the end of your history in one direction
3. swipe swipe swipe to the end of your history in the other direction
4. repeat until you crash.

Results: 

Crash in [@ CoreFoundation@0x884e ] or [@ libobjc.A.dylib@0xb390 ]

Tested on Mac OS X 10.7 (Lion) using a 3.06 GHz Core i3 iMac with a Magic Trackpad
Comment 1 Steven Michaud [:smichaud] (Retired) 2011-08-12 13:50:33 PDT
The addresses Asa gave me (and which which it's easy for to me to reproduce this bug) are:

www.techcrunch.com
www.engadget.com
slashdot.org
techmeme.com/river
news.ycombinator.com
Comment 2 Steven Michaud [:smichaud] (Retired) 2011-08-12 13:51:29 PDT
As Asa pointed out, you don't crash if you use the Back and Forward buttons to switch between these sites.
Comment 3 Steven Michaud [:smichaud] (Retired) 2011-08-12 13:53:57 PDT
I'm going to see what I can do about this.

But it'll take at least several days, and there may not be anything I can do.  I suspect this is an Apple bug.
Comment 4 Steven Michaud [:smichaud] (Retired) 2011-08-12 13:56:15 PDT
Here are a couple of Asa's crash stacks, with their symbols translated:

https://crash-stats.mozilla.com/report/index/bp-7c0602f1-5fec-4e49-bbf9-e946d2110812

0 	CoreFoundation      CoreFoundation@0x884e
1 	CoreFoundation      CoreFoundation@0x30d1f
2 	libobjc.A.dylib     libobjc.A.dylib@0xd2c5
3 	libobjc.A.dylib     libobjc.A.dylib@0xd4f8
4 	libobjc.A.dylib     libobjc.A.dylib@0xf03b
5 	AppKit              AppKit@0x6ceef
6 	AppKit              AppKit@0x6d938
7 	AppKit              AppKit@0x3f2eba
8 	libobjc.A.dylib     libobjc.A.dylib@0xd2c5
9 	libobjc.A.dylib     libobjc.A.dylib@0xd254
10 	CoreFoundation      CoreFoundation@0x30738
11 	AppKit              AppKit@0x6d55b
12 	AppKit              AppKit@0x8e04

0 	CFRelease (in CoreFoundation) + 254
1 	-[__NSArrayM dealloc] (in CoreFoundation) + 303
2 	objc::DenseMap<objc_object*, unsigned long, true, objc::DenseMapInfo<objc_object*>, objc::DenseMapInfo<unsigned long> >::FindAndConstruct(objc_object* const&) (in libobjc.A.dylib) + 75
3 	_objc_rootReleaseWasZero (in libobjc.A.dylib) + 188
4 	(anonymous namespace)::AutoreleasePoolPage::pop(void*) (in libobjc.A.dylib) + 433
5 	_NSPopoverPreProcessLocalEvent (in AppKit) + 46
6 	-[NSApplication sendEvent:] (in AppKit) + 64
7 	-[NSEvent momentumPhase] (in AppKit) + 32
8 	objc::DenseMap<objc_object*, unsigned long, true, objc::DenseMapInfo<objc_object*>, objc::DenseMapInfo<unsigned long> >::FindAndConstruct(objc_object* const&) (in libobjc.A.dylib) + 75
9 	_objc_rootRetain (in libobjc.A.dylib) + 107
10 	-[NSObject retain] (in CoreFoundation) + 40
11 	-[NSApplication _setCurrentEvent:] (in AppKit) + 107
12 	-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 1014

https://crash-stats.mozilla.com/report/index/bp-a14e1854-5fde-4834-88f3-0fe552110812

0 	libobjc.A.dylib     libobjc.A.dylib@0xb390
1 	CoreFoundation      CoreFoundation@0x87ff
2 	CoreFoundation      CoreFoundation@0x30d1f
3 	libobjc.A.dylib     libobjc.A.dylib@0xd2c5
4 	libobjc.A.dylib     libobjc.A.dylib@0xd4f8
5 	libobjc.A.dylib     libobjc.A.dylib@0xf03b
6 	AppKit              AppKit@0x8c485d
7 	AppKit              AppKit@0x6ceef
8 	AppKit              AppKit@0x3f474e
9 	AppKit              AppKit@0x6d938
10 	AppKit              AppKit@0x3f5077
11 	AppKit              AppKit@0x3f49d4
12 	AppKit              AppKit@0x3f474e
13 	libobjc.A.dylib     libobjc.A.dylib@0xd2c5
14 	libobjc.A.dylib     libobjc.A.dylib@0xd254
15 	CoreFoundation      CoreFoundation@0x30738
16 	AppKit              AppKit@0x6d55b
17 	AppKit              AppKit@0x8e04

0 	objc_msgSend_vtable14 (in libobjc.A.dylib) + 16
1 	CFRelease (in CoreFoundation) + 175
2 	-[__NSArrayM dealloc] (in CoreFoundation) + 303
3 	objc::DenseMap<objc_object*, unsigned long, true, objc::DenseMapInfo<objc_object*>, objc::DenseMapInfo<unsigned long> >::FindAndConstruct(objc_object* const&) (in libobjc.A.dylib) + 75
4 	_objc_rootReleaseWasZero (in libobjc.A.dylib) + 188
5 	(anonymous namespace)::AutoreleasePoolPage::pop(void*) (in libobjc.A.dylib) + 433
6 	0x008c485d (in AppKit)
7 	_NSPopoverPreProcessLocalEvent (in AppKit) + 46
8 	__destroy_helper_block_5 (in AppKit) + 18
9 	-[NSApplication sendEvent:] (in AppKit) + 64
10 	__-[NSEvent _initTouches]_block_invoke_1 (in AppKit) + 75
11 	__destroy_helper_block_19 (in AppKit) + 18
12 	__destroy_helper_block_5 (in AppKit) + 18
13 	objc::DenseMap<objc_object*, unsigned long, true, objc::DenseMapInfo<objc_object*>, objc::DenseMapInfo<unsigned long> >::FindAndConstruct(objc_object* const&) (in libobjc.A.dylib) + 75
14 	_objc_rootRetain (in libobjc.A.dylib) + 107
15 	-[NSObject retain] (in CoreFoundation) + 40
16 	-[NSApplication _setCurrentEvent:] (in AppKit) + 107
17 	-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 1014
Comment 5 Steven Michaud [:smichaud] (Retired) 2011-08-12 13:59:06 PDT
I used atos -arch x86_64 [binary] [0xaddress] to obtain these translations.
Comment 6 Steven Michaud [:smichaud] (Retired) 2011-08-12 14:18:23 PDT
> atos -arch x86_64 [binary] [0xaddress]

atos -arch x86_64 -o [path-to-binary] [0xaddress]
Comment 7 Marcia Knous [:marcia - use ni] 2011-08-12 16:38:31 PDT
I tested this using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0a2) Gecko/20110812 Firefox/7.0a2 with similar sites, but haven't yet been able to reproduce the crash. Here are my specifics:

*Using Magic Mouse - not trackpad
*OS X 10.7.2 Build 11C37
*2.8 GHz Intel Core 2 Duo with 8 GB RAM
Comment 8 Steven Michaud [:smichaud] (Retired) 2011-08-12 17:06:23 PDT
> *OS X 10.7.2 Build 11C37

What's this?
Comment 9 Steven Michaud [:smichaud] (Retired) 2011-08-12 17:15:53 PDT
>> *OS X 10.7.2 Build 11C37
>
> What's this?

And where can you get it? :-)
Comment 10 Marcia Knous [:marcia - use ni] 2011-08-12 17:27:34 PDT
Steven: I found it in the iCloud area of the Mac Developer website. You can download it as a .dmg and install. It was just released today.
Comment 11 Benoit Girard (:BenWa) 2011-08-12 18:36:43 PDT
Got the following trace in GDB:

2011-08-12 21:19:02.769 firefox[10028:1207] -_scrollPhase is deprecated for NSScrollWheel. Please use -momentumPhase.
2011-08-12 21:19:27.343 firefox[10028:1207] -_continuousScroll is deprecated for NSScrollWheel. Please use -hasPreciseScrollingDeltas.
2011-08-12 21:19:27.344 firefox[10028:1207] -deviceDeltaY is deprecated for NSScrollWheel. Please use -scrollingDeltaY.
2011-08-12 21:19:27.345 firefox[10028:1207] -deviceDeltaX is deprecated for NSScrollWheel. Please use -scrollingDeltaX.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x00007fff8cec345a in ___trackSwipeWithScrollEvent_block_invoke_1 ()
(gdb) where
#0  0x00007fff8cec345a in ___trackSwipeWithScrollEvent_block_invoke_1 ()
#1  0x00007fff8cb3d6c0 in _NSSendEventToObservers ()
#2  0x00007fff8cb3b939 in -[NSApplication sendEvent:] ()
#3  0x0000000101cd370b in -[GeckoNSApplication sendEvent:] (self=0x10420ab20, _cmd=<value temporarily unavailable, due to optimizations>, anEvent=0x13c553e80) at /Users/bgirard/mozilla/mozilla-central/tree/widget/src/cocoa/nsAppShell.mm:192
Comment 12 Benoit Girard (:BenWa) 2011-08-12 19:53:25 PDT
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x00007fff82ad7390 in objc_msgSend_vtable14 ()
(gdb) where
#0  0x00007fff82ad7390 in objc_msgSend_vtable14 ()
#1  0x00007fff8b417800 in CFRelease ()
#2  0x00007fff8b43fd20 in -[__NSArrayM dealloc] ()
#3  0x00007fff82adb03c in (anonymous namespace)::AutoreleasePoolPage::pop ()
#4  0x00007fff8b4406a5 in _CFAutoreleasePoolPop ()
#5  0x00007fff8adf60d7 in -[NSAutoreleasePool drain] ()
#6  0x00007fff8cad347a in -[NSApplication run] ()
Comment 13 Benoit Girard (:BenWa) 2011-08-12 19:55:48 PDT
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x00007fff8cec3187 in ___trackSwipeWithScrollEvent_block_invoke_1 ()
(gdb) where
#0  0x00007fff8cec3187 in ___trackSwipeWithScrollEvent_block_invoke_1 ()
#1  0x00007fff8cb3d6c0 in _NSSendEventToObservers ()
#2  0x00007fff8cb3b939 in -[NSApplication sendEvent:] ()
#3  0x0000000101cd370b in -[GeckoNSApplication sendEvent:] (self=0x104522a70, _cmd=<value temporarily unavailable, due to optimizations>, anEvent=0x13d013db0) at /Users/bgirard/mozilla/mozilla-central/tree/widget/src/cocoa/nsAppShell.mm:192
Comment 14 Benoit Girard (:BenWa) 2011-08-15 11:10:43 PDT
After fixing the block copy I crash on the following more descriptive trace:

firefox(95686,0x7fff72bdb960) malloc: *** error for object 0x14798ada0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

Breakpoint 1, 0x00007fff8aaed6c0 in malloc_error_break ()
(gdb) where
#0  0x00007fff8aaed6c0 in malloc_error_break ()
#1  0x00007fff8aaed805 in free ()
#2  0x00007fff82adad53 in object_dispose ()
#3  0x00007fff8b43fe16 in -[NSObject dealloc] ()
#4  0x00007fff8ceb9f14 in -[_NSEventObserver dealloc] ()
#5  0x00007fff8b417800 in CFRelease ()
#6  0x00007fff8b43fd20 in -[__NSArrayM dealloc] ()
#7  0x00007fff82adb03c in (anonymous namespace)::AutoreleasePoolPage::pop ()
#8  0x00007fff8b4406a5 in _CFAutoreleasePoolPop ()
#9  0x00007fff8adf60d7 in -[NSAutoreleasePool drain] ()
#10 0x00007fff8cad347a in -[NSApplication run] ()
Comment 15 Markus Stange [:mstange] 2011-08-15 16:30:52 PDT
I have no idea whether that might help any, but have you tried adding *mSwipeAnimationCancelled = YES; mSwipeAnimationCancelled = nil;
to -[ChildView dealloc]?
Comment 16 Markus Stange [:mstange] 2011-08-15 16:34:42 PDT
Actually, scratch that, that ChildView probably won't be released before the window closes.
Comment 17 Markus Stange [:mstange] 2011-08-16 06:59:20 PDT
(In reply to Benoit Girard (:BenWa) from comment #14)
> After fixing the block copy

How?
Comment 18 Benoit Girard (:BenWa) 2011-08-16 07:43:21 PDT
From what I gather this change is required but is no sufficient to fix the crash (don't have a proper patch handy sorry):

in nsChildView.mm
-                         usingHandler:^(CGFloat gestureAmount, NSEventPhase phase, BOOL isComplete, BOOL *stop) {
+                         usingHandler:[^(CGFloat gestureAmount, NSEventPhase phase, BOOL isComplete, BOOL *stop) {
         *stop = YES;
         return;
       }
       if (isComplete) {
         if (gestureAmount) {
           nsSimpleGestureEvent geckoEventCopy(geckoEvent);
           if (gestureAmount > 0) {
             geckoEventCopy.direction |= nsIDOMSimpleGestureEvent::DIRECTION_LEFT;
           } else {
             geckoEventCopy.direction |= nsIDOMSimpleGestureEvent::DIRECTION_RIGHT;
           }
           mGeckoChild->DispatchWindowEvent(geckoEventCopy);
         }
         mSwipeAnimationCancelled = nil;
       }
-    }];
+    } copy]];

Otherwise the block will get release when maybeTrackScrollEventAsSwipe returns. So by copying the blocks we ensure that it will survive when the method return and will be owned by trackSwipeEventWithOptions.
Comment 19 Markus Stange [:mstange] 2011-08-16 08:10:35 PDT
Interesting. I really need to read up on how blocks work.

Another idea (which is probably completely irrelevant again, but might be worth trying): Instead of copying geckoEvent from the scope, how about retaining the NSEvent and creating a fresh geckoEvent from the NSEvent inside the block?

All quick block overviews I've read say something like "blocks handle memory management of non-__block scope variables, don't worry about it, it's magic". Since geckoEvent is a C++ object, maybe something goes wrong there... even though the stack trace doesn't really point to it.
Comment 20 Markus Stange [:mstange] 2011-08-16 08:11:51 PDT
Created attachment 553481 [details] [diff] [review]
retain anEvent
Comment 21 Benoit Girard (:BenWa) 2011-08-16 08:28:57 PDT
(In reply to Markus Stange from comment #19)
> Interesting. I really need to read up on how blocks work.
> 
> Another idea (which is probably completely irrelevant again, but might be
> worth trying): Instead of copying geckoEvent from the scope, how about
> retaining the NSEvent and creating a fresh geckoEvent from the NSEvent
> inside the block?
> 
> All quick block overviews I've read say something like "blocks handle memory
> management of non-__block scope variables, don't worry about it, it's
> magic". Since geckoEvent is a C++ object, maybe something goes wrong
> there... even though the stack trace doesn't really point to it.

At this point I don't believe this is such a far fetch idea :). After seeing the compile segfault by adding a __block keyword I am worried that we are running into a compiler bug.

Have you tried your patch without/with my block copy changes?
Comment 22 Steven Michaud [:smichaud] (Retired) 2011-08-16 08:29:58 PDT
In my tests I find that Benoit's patch (sending 'copy' to the
trackingHandler) doesn't help -- I crash just as often.

I also suspect Markus' patch won't help, either (though I'll also try
it out).

I have a hunch that I'll be playing out over the course of the day:

OS code is 'autorelease'ing some object when it's unsafe to release
it.  This doesn't cause trouble in Safari, or when geckoEventCopy
isn't dispatched from the trackingHandler.  But when geckoEventCopy
*is* dispatched, there are sometimes nested calls to process native
events,	some of which can cause the autorelease pool to be drained.
This can cause the autoreleased object to be released "early".

Right now I suspect (from the evidence of comment #14) that the
autoreleased object is an _NSEventObserver object (or possibly another
object that holds a reference to an _NSEventObserver object -- which
makes sense because an _NSEventObserver object holds a reference to a
block (which is the actual observer).
Comment 23 Steven Michaud [:smichaud] (Retired) 2011-08-16 08:30:52 PDT
I *don't* think the autoreleased object is the trackingHandler block itself -- otherwise Benoit's copy patch would have worked.
Comment 24 Markus Stange [:mstange] 2011-08-16 08:35:55 PDT
(In reply to Benoit Girard (:BenWa) from comment #21)
> Have you tried your patch without/with my block copy changes?

No, as I said, I'll only get back to my 10.7 machine next week, so I can't test anything at the moment. I'm on my Macbook at the moment which runs 10.6 (and downloading Lion will take days over here...).

(In reply to Steven Michaud from comment #22)
As convoluted as it is, that hypothesis sounds pretty plausible :)
Comment 25 Steven Michaud [:smichaud] (Retired) 2011-08-16 08:39:22 PDT
Another thing I've noticed:

Moving the geckoEventCopy constructor to the top of trackingHandler makes it substantially harder to crash.  I assume this is because OS code makes an immutable copy of geckoEvent (to pass to the geckoEventCopy constructor) only on first access, and putting it at the top of trackingHandler ensures that this always happens before maybeTrackSwipeEvent has returned.

Doing this doesn't eliminate *all* crashes, though.
Comment 26 Steven Michaud [:smichaud] (Retired) 2011-08-16 12:20:48 PDT
*** Bug 679438 has been marked as a duplicate of this bug. ***
Comment 27 Colin Barrett [:cbarrett] 2011-08-16 15:37:36 PDT
Have you tried running with Zombies turned on and the Allocations instrument running? That should reveal more about what exactly is being over released, and should even give you a history of all the stacks that retained/released it.
Comment 28 Steven Michaud [:smichaud] (Retired) 2011-08-16 16:05:21 PDT
I've tried running with libgmalloc, but not yet with NSZombies.

libgmalloc didn't tell me anything new.

I've made some progress -- I've found that blocking -[_NSLocalEventObserver dealloc] stops the crashes from happening.  But I haven't yet found exactly why dealloc is sometimes called on an _NSLocalEventObserver object that's already been deleted.

(To stop all the crashes, you also need to do as I suggest in comment #25.)
Comment 29 Steven Michaud [:smichaud] (Retired) 2011-08-16 16:30:38 PDT
> But I haven't yet found exactly why dealloc is sometimes called on
> an _NSLocalEventObserver object that's already been deleted.

Now I've found out more:

+[NSEvent removeMonitor:] can be called	twice on the same
_NSLocalEventObserver object -- which (when it happens) causes a
crash.  I should be able to work around this by hooking calls to
+[NSEvent addLocalMonitorForEventsMatchingMask:handler:] and +[NSEvent
removeMonitor:] and maintaining a local copy of	the sEventObservers
hashtable (containing just those objects of class
_NSLocalEventObserver).

Of course it'd be nice to know the reason for the second call to
+[NSEvent removeMonitor:] -- and I'm going to try to find out.  But
that's not absolutely necessary.
Comment 30 Steven Michaud [:smichaud] (Retired) 2011-08-17 09:05:38 PDT
Created attachment 553794 [details] [diff] [review]
Fix

Here's a patch that fixes this bug!

It does what I described in comment #25 and comment #29. 

A tryserver build should be available in a few hours.

I tested my patch by swiping rapidly between the pages listed in
comment #1.  Here they are again:

www.techcrunch.com
www.engadget.com
slashdot.org
techmeme.com/river
news.ycombinator.com

I also sometimes swiped	back and forth while going through the list.
I did this for three minutes straight without seeing any crashes.
Comment 31 Benoit Girard (:BenWa) 2011-08-17 10:43:44 PDT
(In reply to Colin Barrett [:cbarrett] from comment #27)
> Have you tried running with Zombies turned on and the Allocations instrument
> running? That should reveal more about what exactly is being over released,
> and should even give you a history of all the stacks that retained/released
> it.

I had tried it but didn't get any useful information and it crashes quickly. Instruments has been very useful but it hasn't been stable for me with the latest xcode updates.

(In reply to Steven Michaud from comment #30)
> Created attachment 553794 [details] [diff] [review]

I spent 20 mins trying out this patch with a magic mouse and have seen no crash report.
Comment 32 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-17 11:09:43 PDT
It looks to me were are still missing something. I tried compiling a reduced example of what we have in that file:

struct foo {
  foo();
  foo(const foo&);
  void bar() const;
};
typedef void (^bar)();
void g(bar x);
void f() {
  foo x;
  g(^{x.bar();});
}


I will attach the IL produce by clang, some points that are interesting:

* The copy constructor to foo is called, so "x" in the block is a local copy, no need to do that manually.
* G is passed a stack allocated struct for the block

I am currently building firefox. I will try to reproduce the bug with parts of this patch backed out and see what we get.
Comment 33 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-17 11:10:31 PDT
Created attachment 553841 [details]
il produced by clang
Comment 34 Markus Stange [:mstange] 2011-08-17 11:30:35 PDT
Comment on attachment 553794 [details] [diff] [review]
Fix

(In reply to Steven Michaud from comment #30)
> Created attachment 553794 [details] [diff] [review]
> Fix
> 
> Here's a patch that fixes this bug!

Great!

>+  // Make a dummy call to trackingHandler to ensure that it's called at least
>+  // once before maybeTrackScrollEventAsSwipe:scrollOverflow: returns, so that
>+  // geckoEvent is still in scope when geckoEventCopy is constructed from it.

I still think holding on to the NSEvent would be a better solution than copying the geckoEvent.

(Another thing I've thought of: The retains and releases that I added in attachment 553481 [details] [diff] [review] maybe aren't even necessary because the block should already do that on its own.)
Comment 35 Steven Michaud [:smichaud] (Retired) 2011-08-17 11:49:45 PDT
> I still think holding on to the NSEvent would be a better solution
> than copying the geckoEvent.

Why?  Holding on to the NSEvent could considerably extend its lifetime
(by as much as a second or two), and some of the fields in NSEvent are
pointers, which might in that time have become stale.
Comment 36 Steven Michaud [:smichaud] (Retired) 2011-08-17 12:03:33 PDT
Part of my patch makes the following assumption (quoting from the
patch):

  On first access to a local, non-__block variable in a block's
  enclosing scope, an immutable copy is made of it for use within the
  block.

Rafael tells me that the IL he posted in comment #23 shows this is
wrong.	I'm in the process of figuring out how to read the IL
(http://llvm.org/docs/LangRef.html), and what it means.
Comment 37 Markus Stange [:mstange] 2011-08-17 12:24:14 PDT
(In reply to Steven Michaud from comment #35)
> > I still think holding on to the NSEvent would be a better solution
> > than copying the geckoEvent.
> 
> Why?  Holding on to the NSEvent could considerably extend its lifetime
> (by as much as a second or two), and some of the fields in NSEvent are
> pointers, which might in that time have become stale.

I don't think that can happen; at least the window pointer is strong. The context pointer is probably strong, too, but I couldn't verify that because it was always nil in my tests.

We have other long-lived NSEvents in widget land and haven't had problems with them, for example gLastDragMouseDownEvent, and ChildViewMouseTracker:: sLastMouseMoveEvent which I've just added in bug 678481.
Comment 38 Steven Michaud [:smichaud] (Retired) 2011-08-17 12:27:19 PDT
(In reply to comment #37)

Yes, we may be able to get away with extending the life of the NSEvent.  But I can't understand why you think doing this is "better".  I think it's as ugly as sin :-)
Comment 39 Steven Michaud [:smichaud] (Retired) 2011-08-17 12:34:09 PDT
And it's true that we have several other long-lived NSEvents ... but we actually *need* those :-)
Comment 40 Markus Stange [:mstange] 2011-08-17 12:34:49 PDT
I think it's better because copying Objective-C objects into blocks is well-tread ground and copying C++ objects is not. We also wouldn't need the "call the block while gecko event is still in scope" workaround that your current patch uses (or would we? I don't know).
Comment 41 Steven Michaud [:smichaud] (Retired) 2011-08-17 12:44:52 PDT
That's what copy constructors are for ... and there is one.

But the whole thing may be moot -- I suspect that, once I understand the IL from comment #33 properly, this part of my patch will change quite a lot.
Comment 42 Markus Stange [:mstange] 2011-08-17 13:59:54 PDT
Created attachment 553896 [details]
another small testcase

(In reply to Steven Michaud from comment #41)
> That's what copy constructors are for ... and there is one.

Sure. I'm slowly coming to agree that copying geckoEvent should work just as well.

Here's a small test program that I've used to play around with blocks, and it demonstrates that nothing we're doing here should be problematic.
Output is at the end.
The only notable thing I've found out is that [theBlock copy], which moves the block to the heap, also causes a copy of the CPPStruct object to live on the heap. But in hindsight that's not surprising at all.
Comment 43 Markus Stange [:mstange] 2011-08-17 14:03:07 PDT
I found http://www.friday.com/bbum/2009/08/29/blocks-tips-tricks/ very interesting because it explains why the [block copy] is necessary. It also says "The system provided APIs that take Blocks as arguments will copy said Blocks when warranted. For example, dispatch_async() will copy the passed in Blocks." and I'd find it slightly surprising if trackSwipeEvent... wouldn't make such a copy.
Comment 44 Markus Stange [:mstange] 2011-08-17 14:15:36 PDT
Created attachment 553900 [details]
another small testcase

I've added some more output and found something which worries me:

// CPPStruct(0x7fff5fbfe5b0) copy construction from 0x10040f9b8
// CPPStruct(0x7fff5fbfe5b0) destruction
// block called with x = 0, &e: 0x7fff5fbfe5b0, e.mX: 5, [d retainCount]: 1
// CPPStruct(0x7fff5fbfe5b0) copy construction from 0x10040f9b8
// CPPStruct(0x7fff5fbfe5b0) destruction
// block called with x = 1, &e: 0x7fff5fbfe5b0, e.mX: 5, [d retainCount]: 1

It looks like the block is called with a destroyed object. Transferring this to our specific case, the object that geckoEvent points to inside the block might already have been destroyed.
Comment 45 Colin Barrett [:cbarrett] 2011-08-17 18:56:43 PDT
(In reply to Benoit Girard (:BenWa) from comment #31)
> (In reply to Colin Barrett [:cbarrett] from comment #27)
> > Have you tried running with Zombies turned on and the Allocations instrument
> > running? That should reveal more about what exactly is being over released,
> > and should even give you a history of all the stacks that retained/released
> > it.
> 
> I had tried it but didn't get any useful information and it crashes quickly.
> Instruments has been very useful but it hasn't been stable for me with the
> latest xcode updates.

Assuming you're on Lion, are you using 4.1 or the 4.2 betas? The 4.2 betas are quite bad.

Anyway, zombies at least should work just by setting the NSZombieEnabled environment variable to true. You won't get retain count stack traces though.
Comment 46 Steven Michaud [:smichaud] (Retired) 2011-08-18 13:09:46 PDT
Created attachment 554185 [details] [diff] [review]
Fix rev1 (remove unneeded code)

Just now I found out that only part of my patch	is needed to fix these
crashes.

Because my changes to maybeTrackScrollEventAsSwipe:scrollOverflow:
made it so much harder for me to reproduce these crashes, I assumed
they were still required.  But this isn't true -- the "other part" of
my patch fixes these crashes all by itself.

I tested my patch by rapidly doing two-finger swipes for five minutes
straight -- I didn't crash at all.

The wierdness that Markus found	in how OS block-support code copies
C++ structures still needs to be addressed.  But it turns out to be
irrelevant to this bug's crashes.  I'll have more to say in a later
comment.
Comment 47 Steven Michaud [:smichaud] (Retired) 2011-08-18 13:42:41 PDT
Comment on attachment 554185 [details] [diff] [review]
Fix rev1 (remove unneeded code)

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/2578bdcf32ee
Comment 48 Steven Michaud [:smichaud] (Retired) 2011-08-19 08:58:30 PDT
My patch for this bug is now in mozilla-central nightlies (starting with today's):

ftp://ftp.mozilla.org/pub/firefox/nightly/2011-08-19-03-07-49-mozilla-central/firefox-9.0a1.en-US.mac-shark.dmg

Please try it out, and let us know if you still see any crashes while swiping.
Comment 49 Steven Michaud [:smichaud] (Retired) 2011-08-19 09:03:00 PDT
Apple just released OS X 10.7.1.  But it doesn't fix this bug (testing with yesterday's mozilla-central nightly, which doesn't have my workaround).
Comment 50 Steven Michaud [:smichaud] (Retired) 2011-08-19 09:09:32 PDT
Comment #48 gave the wrong link.  Here's the correct one:

ftp://ftp.mozilla.org/pub/firefox/nightly/2011-08-19-03-07-49-mozilla-central/firefox-9.0a1.en-US.mac.dmg
Comment 51 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-19 13:26:43 PDT
(In reply to Markus Stange from comment #44)
> Created attachment 553900 [details]
> another small testcase
> 
> I've added some more output and found something which worries me:
> 
> // CPPStruct(0x7fff5fbfe5b0) copy construction from 0x10040f9b8
> // CPPStruct(0x7fff5fbfe5b0) destruction
> // block called with x = 0, &e: 0x7fff5fbfe5b0, e.mX: 5, [d retainCount]: 1
> // CPPStruct(0x7fff5fbfe5b0) copy construction from 0x10040f9b8
> // CPPStruct(0x7fff5fbfe5b0) destruction
> // block called with x = 1, &e: 0x7fff5fbfe5b0, e.mX: 5, [d retainCount]: 1
> 
> It looks like the block is called with a destroyed object. Transferring this
> to our specific case, the object that geckoEvent points to inside the block
> might already have been destroyed.


I think you found a bug in gcc-4.2. With clang I get

2011-08-19 16:24:41.391 t[57666:707] RetainCountChangePrinter init
2011-08-19 16:24:41.392 t[57666:707] CPPStruct(0x7fff61ab8538) regular construction
2011-08-19 16:24:41.392 t[57666:707] CPPStruct(0x7fff61ab8530) copy construction from 0x7fff61ab8538
2011-08-19 16:24:41.393 t[57666:707] storing block and queuing for 2 calls
2011-08-19 16:24:41.394 t[57666:707] CPPStruct(0x7f9852912838) copy construction from 0x7fff61ab8530
2011-08-19 16:24:41.394 t[57666:707] RetainCountChangePrinter retain
2011-08-19 16:24:41.395 t[57666:707] RetainCountChangePrinter release
2011-08-19 16:24:41.395 t[57666:707] CPPStruct(0x7fff61ab8530) destruction
2011-08-19 16:24:41.396 t[57666:707] CPPStruct(0x7fff61ab8538) destruction
2011-08-19 16:24:41.895 t[57666:707] block called with x = 0, &e: 0x7f9852912838, e.mX: 5, [d retainCount]: 1
2011-08-19 16:24:42.398 t[57666:707] block called with x = 1, &e: 0x7f9852912838, e.mX: 5, [d retainCount]: 1
2011-08-19 16:24:42.399 t[57666:707] releasing block, retain count before release: 1
2011-08-19 16:24:42.400 t[57666:707] RetainCountChangePrinter release
2011-08-19 16:24:42.400 t[57666:707] RetainCountChangePrinter dealloc
2011-08-19 16:24:42.401 t[57666:707] CPPStruct(0x7f9852912838) destruction
Comment 52 Steven Michaud [:smichaud] (Retired) 2011-08-19 14:22:25 PDT
> I think you found a bug in gcc-4.2. With clang I get

I'm not yet convinced of that.  It seems like a copy of CPPStruct is made and destroyed for every call to NSLog that refers to it (or to its address).  It's definitely destroyed before NSLog "prints" its output, but possibly not before the content of that output is generated.

I've been trying to find a way to pin this down.
Comment 53 Steven Michaud [:smichaud] (Retired) 2011-08-19 14:31:21 PDT
Also, I though that, with XCode 4.1 (on OS X 10.7.X), gcc is a wrapper for clang.
Comment 54 Steven Michaud [:smichaud] (Retired) 2011-08-19 14:36:17 PDT
> Also, I though that, with XCode 4.1 (on OS X 10.7.X), gcc is a wrapper for
> clang.

But Markus has been working on 10.6, so this (even if true) may not be relevant.
Comment 55 Steven Michaud [:smichaud] (Retired) 2011-08-19 15:02:05 PDT
> Also, I though that, with XCode 4.1 (on OS X 10.7.X), gcc is a
> wrapper for clang.

However this may turn out, on OS X 10.7.1 I get the same results as
Rafael when compiling Markus' 2nd testcase (from comment #44) when
compiling with "clang test.mm -lstdc++ -framework Cocoa -o test".
(And the same results as Markus when compiling with "g++ test.mm
-framework Cocoa -o test".)

So even if (in XCode 4.1) g++ is on some level a wrapper for clang,
"g++" and "clang" seem to work at least slightly differently in XCode
4.1.
Comment 56 Steven Michaud [:smichaud] (Retired) 2011-08-19 15:31:45 PDT
Created attachment 554563 [details]
Markus' 2nd testcase with small change

>> I think you found a bug in gcc-4.2.
>
> I'm not yet convinced of that.

*Now* I'm convinced :-(
Comment 57 Steven Michaud [:smichaud] (Retired) 2011-08-19 16:03:35 PDT
The puzzle, though, is why this doesn't seem to cause any crashes.

Maybe the block-support code, though it definitely calls CPPStruct's destructor prematurely, somehow holds on to the memory occupied by CPPStruct.
Comment 58 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 09:02:13 PDT
(In reply to Steven Michaud from comment #53)
> Also, I though that, with XCode 4.1 (on OS X 10.7.X), gcc is a wrapper for
> clang.

No, in xcode 4.1 "gcc" is llvm-gcc and "gcc-4.2" is the old Apple gcc.
Comment 59 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 09:07:17 PDT
(In reply to Steven Michaud from comment #57)
> The puzzle, though, is why this doesn't seem to cause any crashes.
> 
> Maybe the block-support code, though it definitely calls CPPStruct's
> destructor prematurely, somehow holds on to the memory occupied by CPPStruct.

It might just be luck that the memory still points to valid contents. I will run you test on valgrind.
Comment 60 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 09:59:38 PDT
valgrind is not working on 10.7 :-(
Comment 61 Steven Michaud [:smichaud] (Retired) 2011-08-22 10:12:05 PDT
(In reply to comment # 58)

Thanks for the info.

I've also found the following from bug 574346 comment #0:

> Note that LLVM-GCC is different to Clang:
> GCC-4.2 -> Parser (GCC) / Code Generator (GCC)
> LLVM-GCC -> Parser (GCC) / Code Generator (LLVM)
> Clang -> Parser (Clang-LLVM) / Code Generator (LLVM)

> valgrind is not working on 10.7 :-(

You might want to bug Julian Seward :-)

By the way, I've been trying to get Apple's compilers (gcc, llvm-gcc or clang) to dump C/C++ code showing how blocks are implemented, but I haven't been able to figure it out.

Do you know a way to do this?

I can get gcc to dump assembler and "RTL" ... but both are more difficult to read :-(
Comment 62 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 10:38:15 PDT
> You might want to bug Julian Seward :-)

I did :-)
 
> By the way, I've been trying to get Apple's compilers (gcc, llvm-gcc or
> clang) to dump C/C++ code showing how blocks are implemented, but I haven't
> been able to figure it out.
> 
> Do you know a way to do this?
> 
> I can get gcc to dump assembler and "RTL" ... but both are more difficult to
> read :-(

The example I posted is llvm IL, so you can use llvm-gcc or clang. The clang output should be easier to read in general and in the case of blocks (or other new features) more likely to be correct. What you want is

clang++ -S -emit-llvm -o test.ll test.cpp
or
g++ -S -emit-llvm -o test.ll test.cpp

I normally also add -Os to get a "cleaned up" IL. At -O0 the IL is quiet verbose.
Comment 63 Steven Michaud [:smichaud] (Retired) 2011-08-22 10:54:17 PDT
I should have made my question more explicit:

I'm trying to find out where the bug is that causes the CPPStruct destructor to be called prematurely using g++ on 10.6 and 10.7.  It might be in the C/C++ code that Apple's compilers (presumably) generate to support blocks, or it might be in the compilers themselves.  That's why I want to see this C/C++ code, if possible.

Or maybe Apple's block support is generated at a lower level (in RTL using "traditional" gcc/g++ or IL using clang/clang++).

I'll keep digging.
Comment 64 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 14:20:41 PDT
I see.

Since both llvm-gcc (the one in xcode 4.1) and Apple's gcc 4.2 get it wrong, it is very likely that it is the frontend that is producing invalid gimple.

I tried a simple testcase:

-----------------
struct foo {
  foo();
  ~foo();
  foo(const foo&);
  void bar() const;
};
typedef void (^bar)();
void g(bar x);
void f() {
  foo x;
  g(^{x.bar();});
}
---------------

With the llvm-gcc include in xcode 4.1 (a.k.a. gcc), I get the following:

...
  call void @_ZN3fooD1Ev(%struct.foo* %x) nounwind
  call void @_ZNK3foo3barEv(%struct.foo* %x) nounwind
....

note that foo::bar is being called with a destructed object. No such sequence appears with clang.
Comment 65 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 15:36:13 PDT
In the full file, the IL shows a call to the destructor:

call void @_ZN20nsSimpleGestureEventD1Ev(%struct.nsSimpleGestureEvent* %geckoEvent) ssp


Followed by a use (the construction of the copy):

  call void @_ZN20nsSimpleGestureEventC1ERKS_(%struct.nsSimpleGestureEvent* %geckoEventCopy, %struct.nsSimpleGestureEvent* %geckoEvent) ssp

What the constructor does in the end is call _ZN13nsCOMPtr_baseD2Ev on the various members. This destructor is defined in nsCOMPtr.o. 

That destructor checks it the raw pointer is null and call a virtual method (Release) in it.

In summary, while the base memory is not being freed (the storage is part of the block context), all the destructors are and they include virtual calls, so this looks really scary.
Comment 66 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 15:38:56 PDT
Created attachment 554981 [details]
IL produce by llvm-gcc (the "gcc" in xcode 4.1) for sChildView.mm
Comment 67 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 15:47:26 PDT
all the destructors are *called*...

I can try building the current llvm-gcc trunk to check if it is fixed, but I am not sure it is worth it.

gcc is gone from xcode 4.2 (I will double check if llvm-gcc is too).
Comment 68 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-23 09:25:59 PDT
The "llvm-gcc/gcc can call the destructor too early" bug has been report to Apple. The rdar number is 10007196.
Comment 69 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-23 15:03:27 PDT
The gcc crashing bug is now rdar 10010257. The reduced testcase is:

struct nsSimpleGestureEvent {
  nsSimpleGestureEvent(int);
};
@implementation ChildView - (void)maybeTrackScrollEventAsSwipe:(void *)anEvent   {
__attribute__((__blocks__(byref))) nsSimpleGestureEvent geckoEvent(1);
}
@end
Comment 70 Steven Michaud [:smichaud] (Retired) 2011-08-23 15:25:35 PDT
(In reply to comment #65)

> _ZN20nsSimpleGestureEventD1Ev (aka
> nsSimpleGestureEvent::~nsSimpleGestureEvent())
> ...
> What the constructor does in the end is call _ZN13nsCOMPtr_baseD2Ev
> on the various members. This destructor is defined in nsCOMPtr.o.
>
> That destructor checks it the raw pointer is null and call a virtual
> method (Release) in it.

This doesn't make much sense.

If you change "what the constructor ..." to "what the destructor ..."
it's still not very clear, but it does start to make sense.

The problem is that an nsSimpleGestureEvent object, while not itself
an XPCOM object, has a number of member variables that *are* XPCOM
objects:

nsEvent.userType
nsEvent.target
nsEvent.currentTarget
nsEvent.originalTarget
nsGUIEvent.widget
nsMouseEvent_base.relatedTarget

Each of these objects gets destroyed (its destructor is called) when
its parent destructor is called.  But the entire object is stored
inside the parent (an nsCOMPtr<> is not a pointer).  And while
Release() is called on nsCOMPtr_base.mRawPtr (which *is* a pointer) if
it's non-null, this doesn't necessarily mean that the object pointed
to by mRawPtr is also released (if other objects also hold references
to it).

The trackingHandler block's prologue uses nsSimpleGestureEvent's copy
constructor to make a copy (of another good copy) of geckoEvent before
any of "our own" block code runs:  First it uses "alloca" to allocate
sizeof(nsSimpleGestureEvent) bytes on the stack (which won't go out of
scope until the end of the trackingHandler block).  Then it calls the
nsSimpleGestureEvent copy constructor to initialize the memory it just
allocated.  To make the following discussion easier to understand,
I'll call this local copy prologueGeckoEvent.

But (and here's Apple's bug), immediately afterwards the prologue
calls nsSimpleGestureEvent's destructor (and therefore also all of its
superclasses' destructors) on prologueGeckoEvent.  This doesn't free
the space occupied by prologueGeckoEvent, or by any of its nsCOMPtr
member variables.  But each nsCOMPtr's destructor does get called, and
Release() does get called on each non-null nsCOMPtr_base.mRawPtr
(which stays non-null).

But even this doesn't, by itself, cause any trouble.  It doesn't cause
any of the XPCOM objects pointed to by prologueGeckoEvent's nsCOMPtrs
to be released prematurely -- since the good copy from which
prologueGeckoEvent was created still exists, and stays "good" until
after the block is called for the last time.  And so each of these
nsCOMPtrs' mRawPtrs remains valid -- whether or not it's null.
Comment 71 Steven Michaud [:smichaud] (Retired) 2011-08-23 16:12:52 PDT
Comment #70 shows (I think) why	Apple's "premature destructor" bug
doesn't cause current code (specifically my patch for bug 668953) to
crash, and why it's safe to continue using it.

But we definitely don't want to start using "blocks" freely in our OS
X code until we move to	always and only using clang (instead of
gcc/g++ or llvm-gcc/llvm-g++).

I breathe a sigh of relief that the destructors for nsEvent and its
subclasses mostly don't do anything, and that nsCOMPtr objects behave
the way they do.
Comment 72 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-24 06:38:42 PDT
> This doesn't make much sense.
> 
> If you change "what the constructor ..." to "what the destructor ..."
> it's still not very clear, but it does start to make sense.

Yes, sorry. I was typing while reading the IL. The summary is that the only interesting thing the destructor does is call the virtual Release in the member variable that are smart pointers.

> The trackingHandler block's prologue uses nsSimpleGestureEvent's copy
> constructor to make a copy (of another good copy) of geckoEvent before
> any of "our own" block code runs:  First it uses "alloca" to allocate
> sizeof(nsSimpleGestureEvent) bytes on the stack (which won't go out of
> scope until the end of the trackingHandler block).  Then it calls the
> nsSimpleGestureEvent copy constructor to initialize the memory it just
> allocated.  To make the following discussion easier to understand,
> I'll call this local copy prologueGeckoEvent.

correct.

> But (and here's Apple's bug), immediately afterwards the prologue
> calls nsSimpleGestureEvent's destructor (and therefore also all of its
> superclasses' destructors) on prologueGeckoEvent.  This doesn't free
> the space occupied by prologueGeckoEvent, or by any of its nsCOMPtr
> member variables.  But each nsCOMPtr's destructor does get called, and
> Release() does get called on each non-null nsCOMPtr_base.mRawPtr
> (which stays non-null).

Check.

> But even this doesn't, by itself, cause any trouble.  It doesn't cause
> any of the XPCOM objects pointed to by prologueGeckoEvent's nsCOMPtrs
> to be released prematurely -- since the good copy from which
> prologueGeckoEvent was created still exists, and stays "good" until
> after the block is called for the last time.  And so each of these
> nsCOMPtrs' mRawPtrs remains valid -- whether or not it's null.

I haven't investigated the code past the calls to Release. Happy to know it is safe in the current state. I am still a bit nervous about it, since it is an unstable situation. A change to the destructor of the class, of a base class or of a member can make this unsafe :-(
Comment 73 Steven Michaud [:smichaud] (Retired) 2011-08-24 08:38:46 PDT
> A change to the destructor of the class, of a base class or of a
> member can make this unsafe :-(

Yes :-(

The only reason I want to use blocks now is to fix bug 668953, for
which there's very high demand.  Apple's new [NSEvent
trackSwipeEventWithOptions:...] uses blocks, and it seems to be the
best way to provide support for two-finger swipes on OS X Lion.

The nsCOMPtr code seems quite stable, and nsEvent and its subclasses
mostly don't have explicit destructors.  So I think there's a good
chance we can get away with continuing to use [NSEvent
trackSwipeEventWithOptions:...] for the next year or so, despite
Apple's bug.

However, since the premature destructor bug doesn't exist in clang, it
seems unlikely that Apple's going to fix it in gcc/g++ or
llvm-gcc/llvm-g++.  So if we want to keep using [NSEvent
trackSwipeEventWithOptions:...] (or blocks in general), we'll almost
certainly need to switch to using clang on OS X.

As far as I know, the biggest obstacle to switching to clang is that
we need to continue supporting OS X Leopard (10.5.X) for a while.  But
Apple will soon have released its last security update for Leopard (if
it hasn't done so already), and we'll probably want to drop our own
support (on the trunk) within the next 12 months or so.

So maybe we should plan to switch to clang at the same time we drop
support for Leopard.
Comment 74 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-24 08:54:12 PDT
(In reply to Steven Michaud from comment #73)
> > A change to the destructor of the class, of a base class or of a
> > member can make this unsafe :-(
> 
> Yes :-(
> 
> The only reason I want to use blocks now is to fix bug 668953, for
> which there's very high demand.  Apple's new [NSEvent
> trackSwipeEventWithOptions:...] uses blocks, and it seems to be the
> best way to provide support for two-finger swipes on OS X Lion.
>
> The nsCOMPtr code seems quite stable, and nsEvent and its subclasses
> mostly don't have explicit destructors.  So I think there's a good
> chance we can get away with continuing to use [NSEvent
> trackSwipeEventWithOptions:...] for the next year or so, despite
> Apple's bug.

Agreed, hopefully we will be able to switch to clang before this breaks.

> However, since the premature destructor bug doesn't exist in clang, it
> seems unlikely that Apple's going to fix it in gcc/g++ or
> llvm-gcc/llvm-g++.  So if we want to keep using [NSEvent
> trackSwipeEventWithOptions:...] (or blocks in general), we'll almost
> certainly need to switch to using clang on OS X.

Yes, once we move out of vc 2005 gcc 4.2 will also be our oldest compiler.

> As far as I know, the biggest obstacle to switching to clang is that
> we need to continue supporting OS X Leopard (10.5.X) for a while.  But
> Apple will soon have released its last security update for Leopard (if
> it hasn't done so already), and we'll probably want to drop our own
> support (on the trunk) within the next 12 months or so.

Clang works just fine targeting 10.5. The obstacles I know for a full switch are

* Clang -O0 produces programs that use too much stack, which causes oranges in the bots. We want to switch the bots to use optimizations anyway, so this should be fixed soon. (bug 669953)

* Infrastructure. We need to set up bots running clang. We already have a clang package installed, so we need to figure out with releng how hard it is (bug 629459).

> So maybe we should plan to switch to clang at the same time we drop
> support for Leopard.

I agree we need both, but they can be independent :-)
Comment 75 Steven Michaud [:smichaud] (Retired) 2011-08-24 09:21:12 PDT
> Clang works just fine targeting 10.5.

Can you do clang builds while using the 10.5 SDK?

Work's in progress at bug 674655 to switch to using the 10.6 SDK even for builds that are supposed to be runnable on 10.5.  And so far no problems have (apparently) been found.  But, like Josh, I'm skeptical -- Apple has a history of neglecting backwards compatibility, and sometimes there's no substitute for linking to the actual target libraries (in this case the 10.5 libraries).  We can't be sure this will work until it's undergone a *lot* of testing, including testing by users.
Comment 76 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-24 10:27:38 PDT
I have done builds some time ago using the 10.5 SDK. In fact, I did try build on the bots, so they use our current 10.6/10.5 sdk split for 64 and 32 bits.

I will try sending a new try request.
Comment 77 Steven Michaud [:smichaud] (Retired) 2011-08-24 11:08:58 PDT
I've opened bug 681694 on the Apple premature destructor bug.
Comment 78 Steven Michaud [:smichaud] (Retired) 2011-08-24 13:41:45 PDT
*** Bug 673456 has been marked as a duplicate of this bug. ***
Comment 79 Scoobidiver (away) 2011-08-24 14:46:08 PDT
The crash signature of bug 673456 is #2 top browser crasher in 7.0b1 and #1 on Mac OS X only.
Comment 80 Scoobidiver (away) 2011-08-24 14:58:25 PDT
*** Bug 681781 has been marked as a duplicate of this bug. ***
Comment 81 Marcia Knous [:marcia - use ni] 2011-08-26 16:26:56 PDT
Steven: [@ ___trackSwipeWithScrollEvent_block_invoke_1 ] is another new signature showing up on the 7.0 radar - I assume it may be related to this bug?
Comment 82 Steven Michaud [:smichaud] (Retired) 2011-08-29 10:20:31 PDT
> Steven: [@ ___trackSwipeWithScrollEvent_block_invoke_1 ] is another new signature
> showing up on the 7.0 radar - I assume it may be related to this bug?

Yes, I'm sure it is.
Comment 83 Asa Dotzler [:asa] 2011-08-29 14:48:04 PDT
Did we back this out for Aurora and Beta? The status is still "affected" for those trains.
Comment 84 Steven Michaud [:smichaud] (Retired) 2011-08-29 14:54:23 PDT
> Did we back this out for Aurora and Beta?

Not this patch.  We backed out the patch for bug 668953, which triggered this bug's crashes (because it introduced Apple's blocks to the tree).

> The status is still "affected" for those trains.

It shouldn't be.
Comment 85 Scoobidiver (away) 2011-08-29 23:31:24 PDT
(In reply to Steven Michaud from comment #84)
> > The status is still "affected" for those trains.
> It shouldn't be.
It's me that marked this bug as affected for 7.0 and 8.0, but after looking at crash reports, I see no occurences of these crash signatures in 7.0b2 or 8.0a2:
https://crash-stats.mozilla.com/report/list?signature=libobjc.A.dylib@0xb390
https://crash-stats.mozilla.com/report/list?signature=CoreFoundation@0x884e
https://crash-stats.mozilla.com/report/list?signature=objc_msgSend_vtable14%20|%20CFRelease%20|%20-[__NSArrayM%20dealloc]
https://crash-stats.mozilla.com/report/list?signature=___trackSwipeWithScrollEvent_block_invoke_1

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