Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imagickpixeliterator probably shouldn't segfault #530

Open
Danack opened this issue Jan 3, 2022 · 0 comments
Open

imagickpixeliterator probably shouldn't segfault #530

Danack opened this issue Jan 3, 2022 · 0 comments

Comments

@Danack
Copy link
Collaborator

Danack commented Jan 3, 2022

Danack
imagickpixeliterator_object_handlers.get_gc = php_imagickpixeliterator_get_gc;
is there anything other than that ^^ required to get the gc hook to be triggered when an object can be garbage collected?
no_idea_what_I'm_doing_dog.jpg

NikiC
NikiC
9:58 PM
@Danack nope

Danack
Danack
So php_imagickpixeliterator_get_gc should be called when the imagickpixeliterator object goes out of scope, before its destructor is called?

NikiC
NikiC
no
get_gc is called when the cycle GC runs
if you want to do something before the destructor is called you can override the dtor_obj handler
Though I'm not sure when you would ever need to do that

Danack
Danack
Ok, I'll ask what I should be doing then. I have a ImaigckPixelIterator object that has a method getNextIteratorRow().
which returns an array of ImagickPixel objects.
The ImagickPixelIterator needs to not be destroyed, until all of the ImagickPixel objects in that returned array also can be destroyed.
So where can I tell PHP, this object is still needed, if there are references to those other objects stil live?

NikiC
NikiC
Why is the object still needed?

Danack
Danack
Long story - bugs.php.net/bug.php?id=73840
I think internally ImageMagick is freeing all of the pixel objects that were created by a PixelIterator object when the iterator is freed.

NikiC
NikiC
10:04 PM
okay
So the iterator owns the pixels?
And you are creating objects referencing pixels owned by the iterator (rather than by the objects)?

Danack
Danack
That is my understanding.

NikiC
NikiC
In that case the pixels need to reference the iterator
Though the design sounds distinctly weird

Danack
Danack
So, add just add a reference to the iterator object, when a 'child' pixel object is created? And decrement when the pixel object is destroyed?

NikiC
NikiC
yes

Danack
Danack
Though the design sounds distinctly weird

NikiC
NikiC
10:07 PM
and add get_gc handlers, as you now have cycles
@Danack I'd first make sure that this is really how pixeliterator works, and isn't just an artifact of some other issue

Danack
Danack
Yes, what could be better than reading the source code

Wes
Wes
@trowski maybe you can help me, can i ensure that N methods from 2+ different mocks are called in the order i define? i want do do: mock A's foo() will be called, then mock B's bar() will be called, then mock A's foo() will be called again, in this order
it seems each mock has its own queue

Danack
Danack
actually - that was easy to find. Destroying the iterator does definitely destroy the pixel_wands it had created: github.com/ImageMagick/ImageMagick/blob/master/MagickWand/…
And there is a hack to prevent a double-free for those pixel objects in Imagick: github.com/mkoppanen/imagick/blob/master/imagick.c#L2892-L2893
@nikic just to clarify one thing, is it actually a cycle? The pixel iterator object in PHP, isn't currently aware of the pixel objects, so there's only a single direction of references, rather than a cycle, right?

NikiC
NikiC
10:22 PM
@Danack in that case, not a cycle

Danack
Danack
cool, thanks.

@Danack Danack mentioned this issue Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant