verygoodsecurity/vgs-collect-ios

[ISSUE] Memory leak in VGSCollect

Closed this issue · 2 comments

VGSCollect keeps an reference to text fields it is handling:

VGSTextFields try to unregister on deinit:

But this never happens, because VGSCollect is holding a strong reference to the fields, so they will never be deinited. As a consquence, VGSCollect can not be reused since it keeps any VGSTextFields alive forever.

To fix this, there are two options:

  1. Change this architecture. It is a bit questionable to keep an array of references around like this.

  2. If this is not an option, the array needs to not hold strong references. In this case, it might be useful to use a weak reference wrapper like the following:

    private struct WeakReference<T: AnyObject> {
    weak var value: T?
    init(_ value: T) { self.value = value }
    }

And then change var elements = [VGSTextField]() to var elements = [WeakReference<VGSTextField>]().

Thank's for your findings @DagAgren . At the beginning there was idea to have one VGSCollect instance per ViewController so textfields live as long as VGSCollect instance. Also there were some cases when we should keep strong reference to textfields, since on sendData(_:) request VGSCollect grab the data directly from textfields. Anyway we will check if we can improve it.

In v1.7.1 we released functionality that allows to unsubscribe VGSTextFields any time you need it, so you can use it as:

/// Unsubscribe single VGSTextField
vgsCollect.unsubscribeTextField(textField)

//// Unsubscribe all related VGSTextFields
vgsCollect.unsubscribeAllTextFields()

As mentioned before, there are use-case when data from VGSTextFields is collected on multiple screens(e.g. pop-up views) and should be send all together at one point. VGSCollect grabs the data from textfields just before sendData() request, that's why we need to keep strong references to fields. In case when VGSCollect instance is deinited, VGSTextFields will be deinited too(if no other strong references pointing to textfields).

In your case if you want to reuse VGSCollect instance, I would recommend to unsubscribe textfields manually when needed.