matomo-org / matomo-sdk-ios

Matomo iOS, tvOS and macOS SDK: a Matomo tracker written in Swift
MIT License
388 stars 164 forks source link

Crash with 4.3.0 version #203

Closed luohui8891 closed 6 years ago

luohui8891 commented 6 years ago

Hi guys,

I upgrade piwik-sdk-ios to 4.3.0 in our new version and public to external beta testing. But I got two new kinds of crash reports from beta users. Here is the crash 1 and crash 2 Looks like it occurred when the SDK dispatch to send request.

Any idea?

BTW, I will upgrade sdk to 4.4.0 in the new beta build.

brototyp commented 6 years ago

Hi @luohui8891, thanks for the bug report. I will investigate and will get back with you.

luohui8891 commented 6 years ago

@brototyp here is a new one in 4.4.0. I think they are same issue about Event memory management.

luohui8891 commented 6 years ago

Any update?

brototyp commented 6 years ago

Hi @luohui8891, badly no. I haven't had any spare time. I will find some on the weekend.

brototyp commented 6 years ago

Hi @luohui8891, I just noticed you are using the 4.3.0, while the 4.4.0 is available. In 4.4.0 we fixed some issues with the SessionDispatcher. Do you think it is possible to update to 4.4.0?

I will check out the 4.4.0 crash now.

Additionaly: Is there a log (possibly of just the Piwik lines)? And can you show me the 11 TapatalkFree 0x10111eefc -[TKPiwikTrackerManager trackForum:screen:] (TKPiwikTrackerManager.m:49) function of the last crash?

luohui8891 commented 6 years ago

Since I saw the crashes in 4.3.0, I did following:

  1. Create this issue to ask for help.
  2. Upgrade Piwik iOS SDK to 4.4.0 and release the new build to external beta. But this crash still appear.

And here is the code of this function you mentioned.

- (void)trackForum:(TKForumManager*)forumManager screen:(NSString *)screen{
    NSString * piwikID = forumManager.ForumConfigModel.sitePiwikID;
    if (piwikID.length == 0 || [piwikID isEqualToString:@"0"]) {
        return;
    }
    TKPiwikTracker * tracker = [self trackerForPiwikID:piwikID];
    if (tracker) {
        // Here is Line 49
        [tracker trackScreenWithScreen:screen url:[NSURL URLWithString:forumManager.ForumURL]];
    } else {
        dispatch_async(dispatch_get_main_queue(), ^{
            TKPiwikTracker * tracker = [[TKPiwikTracker alloc] initWithPiwikID:piwikID];
            [tracker trackScreenWithScreen:screen url:[NSURL URLWithString:forumManager.ForumURL]];
            self.cache[piwikID] = tracker;
        });
    }
}

BTW, the crash in 4.4.0 I mentioned is not new in 4.4.0 I think. Because I saw some similar crash report in the our build with Piwik 4.3.0.

brototyp commented 6 years ago

Hi @luohui8891, I am still struggling to reproduce this issue. I am suspecting this issue only appears when called from Objective-c or in a mixed environment. I will digg further into that.

luohui8891 commented 6 years ago

Thanks. I downgrade the SDK to my forked repo based on 4.0.0 in our Application. Tell me if you got any fix, I will help you to confirm in our beta build.

flaushi commented 6 years ago

Yes, I am calling from Objective-C and do encounter this error too!

luohui8891 commented 6 years ago

Hi @brototyp by reviewing code, I think this issue is caused by the MemoryQueue. It is not thread-safe. Should we add some protection?

brototyp commented 6 years ago

Hi @luohui8891, thanks for that Idea. My initial reaction was "That can't be, we are moving all the calls to the main thread." but actually that is not right. All (indirect) calls to internal func queue(event: Event) { that are sent in the background will use the MemoryQueue from the background. I will digg into that tonight and see weather we should make the MemoryQueue thread save or move all calls to it to the main thread.

flaushi commented 6 years ago

The problem is that I cannot force the crash, means I have to ship a new version to all users to see an effect.

I have integrated your PR now temporally, and no crashes happen on my debug devices.

I am not happy with releasing an app version with non-release versions of libraries.

But anyway, thank you already now for your great support!

flaushi commented 6 years ago

I am submitting a new version that includes this bugfix to the store now. I'll report on the effect. Thanks @brototyp ! Merry xmas

luohui8891 commented 6 years ago

@brototyp because of the crash issue, we still using the Piwik SDK from on my forking repo. Last week, we did a fix and released it in a new version. How we have released 20% of this version and didn't see the crash anymore. I think the fix is better then we are moving all the calls to the main thread., but still not the best. We can submit a PR of this fix to you if you'd like.

brototyp commented 6 years ago

Hi @flaushi, thanks for the followup! Let's hope it fixes the issue. Hi @luohui8891, I started with a similar implementation but changed it because synchronizing it in a certain thread might lead to issues, if multiple threads would use the queue. For example: Thread 1 gehts 20 Events, Thread 2 gets 20 Events, both dispatch those Events, both remove these Events. Those issues aren't solved in my solution, but they don't seem to be fixed in the MemoryQueue. The PiwikTracker makes sure those can never happen. What do you think?

flaushi commented 6 years ago

So, after 10 days, I can confirm that the crashes do not happen anymore. Thanks!

brototyp commented 6 years ago

Awesome. Thanks for the followup! I will merge the PR and close this issue. Thanks for your help @flaushi and @luohui8891!