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

Session: Crash because trackersBlocked is empty #1624

Closed
colintheshots opened this issue Dec 28, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@colintheshots
Copy link
Contributor

commented Dec 28, 2018

The Focus 8.0.4 release has a major crash affecting about 5% of sessions. WebView and Chrome were crashing inside of Focus with an NDK crash on the callback which counts blocked trackers. I reached out to a Google Chrome Android engineer to figure out how to get the full stack trace and he provided it to me.

The crash is occurring because the list of blockedTrackers is still empty, but we're asking for the last element in the Session component. Session.kt:219. It looks like there's a race condition where it assumes that the underlying collection property value is not empty in a Delegates.observable callback. The same mistaken assumption is also made a few lines later at line 230 with findResults.

java.util.NoSuchElementException: List is empty.
at kotlin.collections.CollectionsKt___CollectionsKt.last    (CollectionsKt___CollectionsKt.java:363 )
at mozilla.components.browser.session.Session$$special$$inlined$observable$11$lambda$1.invoke    (Session.java:219 )
at mozilla.components.browser.session.Session$$special$$inlined$observable$11$lambda$1.invoke    (Session.java:22 )
at mozilla.components.support.base.observer.ObserverRegistry.notifyObservers    (ObserverRegistry.java:101 )
at mozilla.components.browser.session.Session.notifyObservers    (Session.java )
at mozilla.components.browser.session.Session.notifyObservers    (Session.java:297 )
at mozilla.components.browser.session.Session.access$notifyObservers    (Session.java:22 )
at mozilla.components.browser.session.Session$$special$$inlined$observable$11.afterChange    (Session.java:72 )
at kotlin.properties.ObservableProperty.setValue    (ObservableProperty.java:41 )
at mozilla.components.browser.session.Session.setTrackersBlocked    (Session.java )
at org.mozilla.focus.session.SessionCallbackProxy.countBlockedTracker    (SessionCallbackProxy.java:89 )
at org.mozilla.focus.webview.TrackingProtectionWebViewClient.shouldInterceptRequest    (TrackingProtectionWebViewClient.java:112 )
at org.mozilla.focus.webview.FocusWebViewClient.shouldInterceptRequest    (FocusWebViewClient.java:148 )
at com.android.webview.chromium.WebViewContentsClientAdapter.shouldInterceptRequest    (WebViewContentsClientAdapter.java:270 )
at org.chromium.android_webview.AwContents$BackgroundThreadClientImpl.shouldInterceptRequest    (AwContents.java:585 )
at org.chromium.android_webview.AwContentsBackgroundThreadClient.shouldInterceptRequestFromNative    (AwContentsBackgroundThreadClient.java:26 )

colintheshots added a commit to colintheshots/android-components that referenced this issue Dec 28, 2018

@csadilek

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

The same mistaken assumption is also made a few lines later at line 230 with findResults.

@colintheshots great find, but that's a weird state. According to the KDocs (on Delegates) this shouldn't even be possible. So, it wasn't even an assumption that we made. For the observable callback they claim that:

the callback which is called after the change of the property is made. The value of the property has already been changed when this callback is invoked.

So, according to the docs trackersBlocked and new should be the same thing here: https://github.com/mozilla-mobile/android-components/blob/master/components/browser/session/src/main/java/mozilla/components/browser/session/Session.kt#L224

colintheshots added a commit to colintheshots/android-components that referenced this issue Dec 28, 2018

@csadilek

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

OK, so we're seeing two different concurrent threads (one for a loading state change which sets trackersblocked to empty) and one for shouldInterceptRequest which blocks a tracker. Our logic here didn't account for that and this seems to happen (more) now on the latest WebView. So, we need to land this fix.

@colintheshots

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2019

This was not happening more with the latest WebView. That can be confirmed by the fact that with 50% of users on 7.0.13 and 50% on 8.0.4, 96% of the crashes were in 8.0.4 and the rest were in Nightly and Beta versions. The browser-session component was not present in any 7.0.x version, but was present in 8.0.4 and all Nightly releases leading up to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.