682445 - Don't copy C++ object into Objective C block; instead, hold on to an Objective C object

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

Attachment: 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)]

> 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]]

(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]]

Attachment: tweaked testcase

Here's the testcase with a little more output, and without the CPPStruct parts.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

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)]

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]]

(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)]

Attachment: testcase with [block copy]

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: 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)]

Attachment: 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)]

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)]

(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)]

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)]

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)]

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)]

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)]

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]]

Added a comment and pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2bce36a017af

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/2bce36a017af