Created attachment 72888 [details]
bt + console logs on master

On pc Debian x86-64 with master sources updated yesterday (commit ac56d9373a66378a04048993ed5aec65cf39f149), I reproduced the crash.

I attached bt + console logs.

Noel/Uray: one for you?

hmm strange one this, bt doesn't really make a whole lot of sense, valgrind seems to indicate that the core is caused by an access to an uno object that has been
destructed, from the stack it seems that uno object has been deleted when the SbiRuntime ( that runs the method ) has been deleted. So... looks to me that

    Dim oRangeAddress As New com.sun.star.table.CellRangeAddress
    getMergedArea = array (oRangeAddress.StartColumn, _
    			oRangeAddress.StartRow, _
			oRangeAddress.EndColumn, _
			oRangeAddress.EndRow)

the actual array elements are pointing ( referencing ) members of oRangeAddress ( instead of being copies ) and that when the oRangeAddress is deleted ( when getMergedArea has completed ) then we have some sort of hanging reference/pointer left that goes boom when accessed ( e.g. oMerged(2) - 0 )


I'd say this has been around for a long time ( at least I have an OOo 3.3 that exhibits this ) No idea how easy or hard to fix this will be ( never really looked in detail at the array handling ) probably we could do with a simpler example to trigger this ( I will need to try for one to look further )

thanks for the report, nice catch, definitely bad behaviour in anycase

I guessed that it had to be something like that because when I stored long values directly it did not cause a problem. Have had difficulty building a debug version that I can run to test in GDB, but can build and run a non-debug version with no problem. 

I will see if I can perform some tests to trigger the problem  when I use a variant variable to try and determine if this is specific to being contained in an array.

Note that in the original problem, the range address was obtained directly from a sheet as part of a larger macro.

It is slow going when I do this by hand rather than in a debugger, but, what I see so far is that we build an Array. I looked in methods1.cxx at RTLFUNC(Array) and I see the following code used to build the array:

    for( sal_uInt16 i = 0 ; i < nArraySize ; i++ )
    {
        SbxVariable* pVar = rPar.Get(i+1);
        SbxVariable* pNew = new SbxVariable( *pVar );
        pNew->SetFlag( SBX_WRITE );
        short index = static_cast< short >(i);
        if ( bIncIndex )
        {
            ++index;
        }
        pArray->Put( pNew, &index );
    }

This takes us to sbxarray.cxx, where we have the following:

void SbxArray::Put( SbxVariable* pVar, sal_uInt16 nIdx )
{
    if( !CanWrite() )
        SetError( SbxERR_PROP_READONLY );
    else
    {
        if( pVar )
            if( eType != SbxVARIANT )
                // Convert no objects
                if( eType != SbxOBJECT || pVar->GetClass() != SbxCLASS_OBJECT )
                    pVar->Convert( eType );
        SbxVariableRef& rRef = GetRef( nIdx );
        if( (SbxVariable*) rRef != pVar )
        {
            rRef = pVar;
            SetFlag( SBX_MODIFIED );
        }
    }
}

And I need to run.

@Andrew so, I hope I can get a chance to look at this in more depth soon, currently quite busy though, I have a sneaky suspicion that this issue has been around for ever which makes its priority a little lower for me than a normal crash. With that in mind thanks for having a look ( please feel free to look into it as far as you can or even provide a patch )

saying all that the code below seems to tally with my suspicions 

(In reply to comment #5)
> It is slow going when I do this by hand rather than in a debugger, but, what
> I see so far is that we build an Array. I looked in methods1.cxx at
> RTLFUNC(Array) and I see the following code used to build the array:
> 
>     for( sal_uInt16 i = 0 ; i < nArraySize ; i++ )
>     {
>         SbxVariable* pVar = rPar.Get(i+1);
>         SbxVariable* pNew = new SbxVariable( *pVar );
I think what is happening here is that we don't get a real copy of the member of the struct at this point, iirc all uno objects are manipulated via introspection so what will get copied here is the SbUnoProperty. Properties are sortof lazy loaded so the property wont get fired ( and the SbxValues populated ) until the point where we actually need to set ( or read ) the value. Of course not only do you need the property itself to access ( e.g. set or get ) the value but you also need to access the parent ( e.g. the uno struct ) Therein lies the problem 'cause when we do try and access the parent it is gone. ( note: this is what I surmise from looking at the code and not from any hard evidence ( so pinch or salt and true verification via debugger etc. needed ) ) 

so if the above 'guess' is true ( and I really think it is ) then maybe indeed we are just hitting some of the inevitable side-effects/limits of how arrays are represented in libreoffice basic, I mean the fact that we store references to the variables and not a copy. Normally reference counting would take care of things but in this case we don't reference count the 'parent' ( e.g. the struct itself ) and I don't think it would make sense to reference count the parent in this case. Perhaps we could special case array initialisation when dealing with members of some struct or object ( don't know what the implications of that would be, would have to think long and hard about that )... already my head is starting to hurt :-)

In BASIC, structs are returned as a copy, and I had assumed that this might be the problem, of course, referencing any value that will no longer exist will cause a problem I assume. Based on this, I assume that if I store the entire struct in the array that I return, I assume that will also cause a problem. Will see if I can verify that. 

Based on your comments, I created a test that looks more like this:

Function getMergedArea()
	Dim oRangeAddress As New com.sun.star.table.CellRangeAddress
	Dim x As Variant
	x = array (oRangeAddress.StartColumn, _
		oRangeAddress.StartRow, _
		oRangeAddress.EndColumn, _
		oRangeAddress.EndRow)
			    			
	Dim y As Long
	y = x(0) + x(1) + x(2) + x(3)
    getMergedArea = x
End Function 

The primary point of interest is that I force the values to be accessed while the parent still exists, then I return the array. Arrays are always copied as a reference (I believe), so I was worried that the variable x might disappear, but this seems to not cause a problem. In other words, the crash is gone if I force a reference. Although it would be a performance hit, I expect that forcing a reference to every object stored into an array might fix the problem. I am just thinking out loud before I head off to work.

I did test adding the entire struct into the array, and at least with my brief test, the crash went away. If the parent structure is referenced counted, then it lives in the array so no crash I assume.

(In reply to comment #7)
> In BASIC, structs are returned as a copy,
but we are talking about arrays here not normal return semantics right? I guess I just miss you point here, sorry
[...]
> Based on this, I assume that if I store the
> entire struct in the array that I return, I assume that will also cause a
> problem.
I would hope not, I would expect the struct ( if stored in the array ) would be reference counted
[...]
> Based on your comments, I created a test that looks more like this:
>
[...]

> The primary point of interest is that I force the values to be accessed
> while the parent still exists, then I return the array. Arrays are always
> copied as a reference (I believe), so I was worried that the variable x
> might disappear

not sure at all what you mean, x is the array, it's either reference counted ( or not ) but either way x should be valid if copied/returned

>, but this seems to not cause a problem. In other words, the
> crash is gone if I force a reference.

    y = x(0) + x(1) + x(2) + x(3)

I would assume that arithmetic operation acting on x(0), x(1)  etc. at the line above doesn't in fact force some reference counting magic but actually forces a broadcast on the SbxVariable ( which will call the uno property foo on the parent struct which is still in scope at this stage ) and the Long values will be populated in the SbxValues part of the SbxVariable. The later accesses of the array slices in the 'crashMe' method will use the 'cached' values of the SbxVariable and thus the problem is avoided ( again this is all assumption on my part, didn't actually debug it )


If I am correct then the easiest way ( again untested ) to side-step this issue would be to do something like

Function getMergedArea()
    Dim oRangeAddress As New com.sun.star.table.CellRangeAddress
    Dim a,b,c,d
    a = oRangeAddress.StartColumn
    b = oRangeAddress.StartRow
    c = oRangeAddress.EndColumn
    d = oRangeAddress.EndRow
    getMergedArea = array ( a, b, c, d )
End Function 

and again if all of the above is correct some thought would be required to determine if indeed such special treatment ( to force the broadcast on member variable when assigning the slices ) could be used as a solution, of course one would additionally need to be sure ( as sure as one can be with basic ) of not introducing new weird/unwanted behaviour too

(In reply to comment #8)
> (In reply to comment #7)
> > In BASIC, structs are returned as a copy,
> but we are talking about arrays here not normal return semantics right? I
> guess I just miss you point here, sorry
> [...]
> > Based on this, I assume that if I store the
> > entire struct in the array that I return, I assume that will also cause a
> > problem.
> I would hope not, I would expect the struct ( if stored in the array ) would
> be reference counted
> [...]

forget all that above, I am just looking at too many other things at once to think straight, structs are indeed iirc returned by copy. but anyway I still believe the root of the problem is as described previously.

assigned to me

Created attachment 93717 [details]
console logs + bt with symbols

On pc Debian x86-64 with master sources updated yesterday, I reproduced the problem.
Just wanted to give an update.

reassigning to default, don't think I will have time to look a this

This bug lost its owner - back to NEW.

Created attachment 115016 [details]
bt with debug symbols

Just an update with master sources updated today.
Anyway, it's similar to the previous bt.

I tried making all SbxVariables reference count their parent, but that's super hard with references getting added and removed to itself during SbxObject ctors.

So I instead just created a reference counted variant for this one use case of in "Array" (https://gerrit.libreoffice.org/#/c/26272/) and afterwards this crashMe() example doesn't crash.

Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9f0997eb167d0ef2193a59d43ab55ea5f13ebaac

Resolves: tdf#59222 Crash in Basic with an array of values...

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

Hi

Verified on windows 7/64 & Version: 5.3.0.0.alpha1+
Build ID: 8a796410ec8f440b4163b15b928347c499da7a8f
CPU Threads: 2; OS Version: Windows 6.1; UI Render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2016-10-20_23:07:21
Locale: fr-FR (fr_FR); Calc: group

No more crash...

Regards
Pierre-Yves