Last Comment Bug 682445 - Don't copy C++ object into Objective C block; instead, hold on to an Objective C object
: Don't copy C++ object into Objective C block; instead, hold on to an Objectiv...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Markus Stange [:mstange] [away until June 27]
:
Mentors:
Depends on:
Blocks: 678607 681694
  Show dependency treegraph
 
Reported: 2011-08-26 15:45 PDT by Markus Stange [:mstange] [away until June 27]
Modified: 2011-09-08 14:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.45 KB, patch)
2011-08-26 15:45 PDT, Markus Stange [:mstange] [away until June 27]
smichaud: review+
Details | Diff | Review
tweaked testcase (4.27 KB, text/plain)
2011-08-29 09:32 PDT, Markus Stange [:mstange] [away until June 27]
no flags Details
testcase with [block copy] (901 bytes, text/plain)
2011-08-29 12:11 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details
IL for that testcase (21.87 KB, text/plain)
2011-08-29 12:13 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details
More explicit testcase (6.59 KB, text/plain)
2011-09-02 12:06 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details

Description Markus Stange [:mstange] [away until June 27] 2011-08-26 15:45:02 PDT
Created attachment 556157 [details] [diff] [review]
v1

See the discussion starting in bug 678607 comment 19 for background.

I still think that holding on to the NSEvent is a better strategy than holding onto the geckoEvent C++ object. Steven, you said that you think having long-lived NSEvents is ugly, but don't we have long-lived event objects either way?
(OK, the geckoEvent object doesn't have any pointers that could become stale, but the ones in the NSEvent aren't a problem either.)

Objective C blocks were designed to be used in conjunction with Objective C objects, and for this case the automatic memory management works out exactly as we want it. The log at the end of attachment 554563 [details] (the RetainCountChangePrinter parts) demonstrates that.

This patch doesn't cause any problems that I'm aware of and defuses the sleeping bomb that is bug 681694.
Comment 1 Steven Michaud [:smichaud] (Retired) 2011-08-29 08:42:06 PDT
> Objective C blocks were designed to be used in conjunction with
> Objective C objects, and for this case the automatic memory
> management works out exactly as we want it. The log at the end of
> attachment 554563 [details] (the RetainCountChangePrinter parts)
> demonstrates that.

Actually we know very little about how blocks handle external
Objective-C objects that are in the same scope -- less than we (now)
do about how they handle C++ structures.  All that's clear from my
tests is that the block accesses a copy of the external Objective-C
object (not the original).  We don't know, for example, what happens
if the copy's original is released before the block tries to access
the copy.

Please extend your testcase from bug 678607 comment #44 to cover this
contingency, and post it here.
Comment 2 Markus Stange [:mstange] [away until June 27] 2011-08-29 09:30:54 PDT
(In reply to Steven Michaud from comment #1)
> All that's clear from my
> tests is that the block accesses a copy of the external Objective-C
> object (not the original).

How did you come to this conclusion? I'm reading the log completely differently:
The block doesn't make a copy, it uses the original object and keeps a strong reference to it. That is, it retains the object when the block is copied to the heap, and it releases it when the block is deallocated.
Comment 3 Markus Stange [:mstange] [away until June 27] 2011-08-29 09:32:19 PDT
Created attachment 556576 [details]
tweaked testcase

Here's the testcase with a little more output, and without the CPPStruct parts.
Comment 4 Steven Michaud [:smichaud] (Retired) 2011-08-29 09:54:39 PDT
I'll write my own testcase to show what I'm talking about.  I should be finished with it today or tomorrow.
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-29 11:44:17 PDT
A minimal testcase:

typedef void (^ftype)(void);
void g(ftype x);
void h(id x);
id foo(void);

void f() {
  id bar = foo();
  g(^{h(bar);});
}

and the IL for f produced by the llvm-gcc in xcode 4.1:

define void @f() optsize ssp {
entry:
  %__block_holder_tmp_1.0 = alloca %struct.__block_literal_1, align 8 ; <%struct.__block_literal_1*> [#uses=7]
  %0 = call %struct..0objc_object* @foo()         ; <%struct..0objc_object*> [#uses=1]
  %1 = getelementptr inbounds %struct.__block_literal_1* %__block_holder_tmp_1.0, i64 0, i32 0 ; <i8**> [#uses=1]
  store i8* bitcast (i8** @_NSConcreteStackBlock to i8*), i8** %1, align 8
  %2 = getelementptr inbounds %struct.__block_literal_1* %__block_holder_tmp_1.0, i64 0, i32 1 ; <i32*> [#uses=1]
  store i32 1107296256, i32* %2, align 8
  %3 = getelementptr inbounds %struct.__block_literal_1* %__block_holder_tmp_1.0, i64 0, i32 2 ; <i32*> [#uses=1]
  store i32 0, i32* %3, align 4
  %4 = getelementptr inbounds %struct.__block_literal_1* %__block_holder_tmp_1.0, i64 0, i32 3 ; <i8**> [#uses=1]
  store i8* bitcast (void (%struct.__block_literal_1*)* @__f_block_invoke_1 to i8*), i8** %4, align 8
  %5 = getelementptr inbounds %struct.__block_literal_1* %__block_holder_tmp_1.0, i64 0, i32 4 ; <%struct.__block_descriptor_withcopydispose**> [#uses=1]
  store %struct.__block_descriptor_withcopydispose* @__block_descriptor_tmp_1.1, %struct.__block_descriptor_withcopydispose** %5, align 8
  %6 = getelementptr inbounds %struct.__block_literal_1* %__block_holder_tmp_1.0, i64 0, i32 5 ; <%struct..0objc_object**> [#uses=1]
  store %struct..0objc_object* %0, %struct..0objc_object** %6, align 8
  %__block_holder_tmp_1.01 = bitcast %struct.__block_literal_1* %__block_holder_tmp_1.0 to void ()* ; <void ()*> [#uses=1]
  call void @g(void ()* %__block_holder_tmp_1.01)
  ret void
}

It looks like no copies of the obj-c object is made.
Comment 6 Markus Stange [:mstange] [away until June 27] 2011-08-29 11:53:18 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #5)
> A minimal testcase:
> 
> typedef void (^ftype)(void);
> void g(ftype x);
> void h(id x);
> id foo(void);
> 
> void f() {
>   id bar = foo();
>   g(^{h(bar);});
> }

I think that's a little too minimal; the interesting part is what happens when the block is copied to the heap (using Block_copy(block) or the equivalent [block copy]).
Comment 7 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-29 12:11:35 PDT
Created attachment 556641 [details]
testcase with [block copy]
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-29 12:13:38 PDT
Created attachment 556642 [details]
IL for that testcase

I find IL produced for obj-c a bit hard to read, but if I read this one correctly, it looks like all the actual copy work is done by the runtime system behind the copy message, in which case the execution test is probably the best option for figuring out what is going on.
Comment 9 Steven Michaud [:smichaud] (Retired) 2011-09-02 12:06:17 PDT
Created attachment 557907 [details]
More explicit testcase

Turns out your patch should be fine, Markus, and this testcase (I
believe) shows it.

When (in -[TestView setupCallsToBlock:numCalls:]) theBlock is copied
to mBlock, the compiler's added block-support code just increments
objcVar's reference count.  This keeps objcVar around after its
reference count is decremented at the end of -[TestView mouseDown:]
(which would otherwise cause objcVar to be deallocated there).
objcVar is only deallocated when mBlock is deallocated -- which shows
that mBlock is somehow holding a reference to it (maybe the
block-support code is holding a reference on behalf of mBlock).

I think all this can be deduced	from the logs produced by my testcase,
without having to understand its translation into IL.
Comment 10 Steven Michaud [:smichaud] (Retired) 2011-09-02 12:08:59 PDT
Comment on attachment 556157 [details] [diff] [review]
v1

You should probably add a comment explaining how anEvent's lifetime is extended by block support code.
Comment 11 Steven Michaud [:smichaud] (Retired) 2011-09-02 12:13:24 PDT
(Following up comment #9)

I forgot to add that my testcase gives the same output whether it's compiled with g++, llvm-g++ or clang.

I test on OS X 10.7.1.
Comment 12 Steven Michaud [:smichaud] (Retired) 2011-09-02 12:17:08 PDT
Here's a stack trace of the call to -[ChangePrinter retain] made while theBlock is being copied to mBlock:

#0  0x0000000100001bcb in -[ChangePrinter retain] ()
#1  0x00007fff9865551e in _Block_object_assign ()
#2  0x0000000100001df9 in __copy_helper_block_1 ()
#3  0x00007fff98655305 in _Block_copy_internal ()
#4  0x00007fff966c6657 in -[NSBlock copy] ()
#5  0x0000000100001699 in -[TestView setupCallsToBlock:numCalls:] ()
#6  0x000000010000189e in -[TestView mouseDown:] ()
#7  0x00007fff8eddd66e in -[NSWindow sendEvent:] ()
#8  0x00007fff8ed75f19 in -[NSApplication sendEvent:] ()
#9  0x00007fff8ed0c42b in -[NSApplication run] ()
#10 0x000000010000135c in main ()
Comment 13 Steven Michaud [:smichaud] (Retired) 2011-09-02 12:22:40 PDT
The stack in comment #12 is what you see when you compile testcase.mm with g++ or llvm-g++.  When you use clang to compile it, you get the following stack (which is only very slightly different):

#0  0x000000010000123a in -[ChangePrinter retain] ()
#1  0x00007fff9865551e in _Block_object_assign ()
#2  0x00000001000016bd in __copy_helper_block_ ()
#3  0x00007fff98655305 in _Block_copy_internal ()
#4  0x00007fff966c6657 in -[NSBlock copy] ()
#5  0x00000001000017a6 in -[TestView setupCallsToBlock:numCalls:] ()
#6  0x00000001000015d7 in -[TestView mouseDown:] ()
#7  0x00007fff8eddd66e in -[NSWindow sendEvent:] ()
#8  0x00007fff8ed75f19 in -[NSApplication sendEvent:] ()
#9  0x00007fff8ed0c42b in -[NSApplication run] ()
#10 0x0000000100001e15 in main ()
Comment 14 Steven Michaud [:smichaud] (Retired) 2011-09-02 12:28:38 PDT
Here are stack traces for the call to -[ChangePrinter dealloc] (when mBlock is destroyed):

When compiled using g++ or llvm-g++:

#0  0x0000000100001a40 in -[ChangePrinter dealloc] ()
#1  0x0000000100001aff in -[ChangePrinter release] ()
#2  0x0000000100001e23 in __destroy_helper_block_1 ()
#3  0x00007fff98655174 in _Block_release ()
#4  0x0000000100001569 in -[TestView callBlock] ()
#5  0x00007fff8dd20b8e in __NSFireDelayedPerform ()
#6  0x00007fff966e2694 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ ()
#7  0x00007fff966e21e6 in __CFRunLoopDoTimer ()
#8  0x00007fff966c2ba1 in __CFRunLoopRun ()
#9  0x00007fff966c2216 in CFRunLoopRunSpecific ()
#10 0x00007fff94ed44ff in RunCurrentEventLoopInMode ()
#11 0x00007fff94edbc21 in ReceiveNextEventCommon ()
#12 0x00007fff94edbaae in BlockUntilNextEventMatchingListInMode ()
#13 0x00007fff8ed10191 in _DPSNextEvent ()
#14 0x00007fff8ed0fa95 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#15 0x00007fff8ed0c3d6 in -[NSApplication run] ()
#16 0x000000010000135c in main ()

When compiled using clang:

#0  0x0000000100001383 in -[ChangePrinter dealloc] ()
#1  0x0000000100001351 in -[ChangePrinter release] ()
#2  0x00000001000016ee in __destroy_helper_block_ ()
#3  0x00007fff98655174 in _Block_release ()
#4  0x0000000100001971 in -[TestView callBlock] ()
#5  0x00007fff8dd20b8e in __NSFireDelayedPerform ()
#6  0x00007fff966e2694 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ ()
#7  0x00007fff966e21e6 in __CFRunLoopDoTimer ()
#8  0x00007fff966c2ba1 in __CFRunLoopRun ()
#9  0x00007fff966c2216 in CFRunLoopRunSpecific ()
#10 0x00007fff94ed44ff in RunCurrentEventLoopInMode ()
#11 0x00007fff94edbc21 in ReceiveNextEventCommon ()
#12 0x00007fff94edbaae in BlockUntilNextEventMatchingListInMode ()
#13 0x00007fff8ed10191 in _DPSNextEvent ()
#14 0x00007fff8ed0fa95 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#15 0x00007fff8ed0c3d6 in -[NSApplication run] ()
#16 0x0000000100001e15 in main ()
Comment 15 Steven Michaud [:smichaud] (Retired) 2011-09-02 14:50:25 PDT
Using the symbols that appear in the stack traces I posted above, I've
managed to track down more information about blocks.

The biggest find (and biggest surprise) is that	Apple's	block support
code is	at least partly	open source:

http://opensource.apple.com/source/libclosure/libclosure-53/

Source code can also be downloaded at:

http://opensource.apple.com/tarballs/libclosure/libclosure-53.tar.gz

The distro contains a "Language Specification for Blocks", which is
also available at:

http://clang.llvm.org/docs/BlockLanguageSpec.txt

I also found that the NSBlock class lives in the CoreFoundation
framework.  It's undocumented, but header information for it and
related classes (e.g. __NSBlockVariable) can be extracted from the
framework using class-dump.
Comment 16 Steven Michaud [:smichaud] (Retired) 2011-09-02 15:12:41 PDT
Apple has also released source code for the additions they've made to gcc and clang to support blocks:

http://opensource.apple.com/tarballs/gcc/gcc-5646.tar.gz
http://opensource.apple.com/tarballs/clang/clang-23.tar.gz

Grep the code for "copy_helper_block" and "destroy_helper_block".

But neither of these distros is available for OS X 10.7.X -- only for 10.6.X.  Which I suppose means that Apple hasn't released the source for their most recent changes to either gcc or clang.  (And there may not be any recent changes to gcc, since Apple is deprecating it in favor of clang.)
Comment 17 Markus Stange [:mstange] [away until June 27] 2011-09-08 06:35:21 PDT
Added a comment and pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2bce36a017af
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-08 14:03:57 PDT
http://hg.mozilla.org/mozilla-central/rev/2bce36a017af

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