Issue 29703555: Issue 6389 - ABP for Samsung Internet Crashes when closed on specific devices (Closed)
DescriptionIssue 6389 - ABP for Samsung Internet Crashes when closed on specific devices
Patch Set 1 #
Total comments: 2
MessagesTotal messages: 12
https://codereview.adblockplus.org/29703555/diff/29703556/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineService.java (right): https://codereview.adblockplus.org/29703555/diff/29703556/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineService.java:41: return Service.START_NOT_STICKY; it's not clear why it happens only on Google Pixel 1. NOT_STICKY requires the service be started with `startService()` each time and i can't see related change (already working this way?).
https://codereview.adblockplus.org/29703555/diff/29703556/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineService.java (right): https://codereview.adblockplus.org/29703555/diff/29703556/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineService.java:41: return Service.START_NOT_STICKY; On 2018/02/21 08:05:35, anton wrote: > it's not clear why it happens only on Google Pixel 1. > NOT_STICKY requires the service be started with `startService()` each time and > i can't see related change (already working this way?). START_NOT_STICK does not trigger the service again until the next explicit call to Context.startService(Intent). See also my comment in the trac ticket.
On 2018/02/21 09:25:55, jens wrote: > https://codereview.adblockplus.org/29703555/diff/29703556/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineService.java > (right): > > https://codereview.adblockplus.org/29703555/diff/29703556/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/EngineService.java:41: > return Service.START_NOT_STICKY; > On 2018/02/21 08:05:35, anton wrote: > > it's not clear why it happens only on Google Pixel 1. > > NOT_STICKY requires the service be started with `startService()` each time > and > > i can't see related change (already working this way?). > > START_NOT_STICK does not trigger the service again until the next explicit call > to Context.startService(Intent). See also my comment in the trac ticket. Yes, i know that. Just wanted to make sure `startService()` is called every time. Also what about the rootcause of the problem? Why it does not happen on other devices?
> Also what about the rootcause of the problem? Why it does not happen on other > devices? I explained it in the comment of the ticket in trac, It is related to Android 8.0.
On 2018/02/21 15:58:15, jens wrote: > > Also what about the rootcause of the problem? Why it does not happen on other > > devices? > > I explained it in the comment of the ticket in trac, It is related to Android > 8.0. It was not mentioned in the ticket that it's Android 8.0-related issue. Anyway this is strange to happen on Google Pixel 1 only. Can we test on other Android 8.0 devices before pushing to production?
On 2018/02/22 05:42:41, anton wrote: > On 2018/02/21 15:58:15, jens wrote: > > > Also what about the rootcause of the problem? Why it does not happen on > other > > > devices? > > > > I explained it in the comment of the ticket in trac, It is related to Android > > 8.0. > > It was not mentioned in the ticket that it's Android 8.0-related issue. > Anyway this is strange to happen on Google Pixel 1 only. > Can we test on other Android 8.0 devices before pushing to production? Yeah, sorry. We already had a discussion in #contentblockers before and I forgot to also mention that discussion in the ticket. Rick did a full round of QA with an APK that contained the fix from this ticket and did not have any crashes or problem at all. But I can ask him explicitly if he used other Android 8 devices.
On 2018/02/22 08:00:13, jens wrote: > On 2018/02/22 05:42:41, anton wrote: > > On 2018/02/21 15:58:15, jens wrote: > > > > Also what about the rootcause of the problem? Why it does not happen on > > other > > > > devices? > > > > > > I explained it in the comment of the ticket in trac, It is related to > Android > > > 8.0. > > > > It was not mentioned in the ticket that it's Android 8.0-related issue. > > Anyway this is strange to happen on Google Pixel 1 only. > > Can we test on other Android 8.0 devices before pushing to production? > > Yeah, sorry. We already had a discussion in #contentblockers before and I forgot > to also mention that discussion in the ticket. > Rick did a full round of QA with an APK that contained the fix from this ticket > and did not have any crashes or problem at all. But I can ask him > explicitly if he used other Android 8 devices. Yes, i'd prefer to test it with other Android 8 devices (not sure if it should block code-review or not).
On 2018/02/22 08:34:37, anton wrote: > On 2018/02/22 08:00:13, jens wrote: > > On 2018/02/22 05:42:41, anton wrote: > > > On 2018/02/21 15:58:15, jens wrote: > > > > > Also what about the rootcause of the problem? Why it does not happen on > > > other > > > > > devices? > > > > > > > > I explained it in the comment of the ticket in trac, It is related to > > Android > > > > 8.0. > > > > > > It was not mentioned in the ticket that it's Android 8.0-related issue. > > > Anyway this is strange to happen on Google Pixel 1 only. > > > Can we test on other Android 8.0 devices before pushing to production? > > > > Yeah, sorry. We already had a discussion in #contentblockers before and I > forgot > > to also mention that discussion in the ticket. > > Rick did a full round of QA with an APK that contained the fix from this > ticket > > and did not have any crashes or problem at all. But I can ask him > > explicitly if he used other Android 8 devices. > > Yes, i'd prefer to test it with other Android 8 devices (not sure if it should > block code-review or not). LGTM. I commented into the ticket stating the reasons why I think this is fine.
On 2018/02/22 17:14:20, diegocarloslima wrote: > On 2018/02/22 08:34:37, anton wrote: > > On 2018/02/22 08:00:13, jens wrote: > > > On 2018/02/22 05:42:41, anton wrote: > > > > On 2018/02/21 15:58:15, jens wrote: > > > > > > Also what about the rootcause of the problem? Why it does not happen > on > > > > other > > > > > > devices? > > > > > > > > > > I explained it in the comment of the ticket in trac, It is related to > > > Android > > > > > 8.0. > > > > > > > > It was not mentioned in the ticket that it's Android 8.0-related issue. > > > > Anyway this is strange to happen on Google Pixel 1 only. > > > > Can we test on other Android 8.0 devices before pushing to production? > > > > > > Yeah, sorry. We already had a discussion in #contentblockers before and I > > forgot > > > to also mention that discussion in the ticket. > > > Rick did a full round of QA with an APK that contained the fix from this > > ticket > > > and did not have any crashes or problem at all. But I can ask him > > > explicitly if he used other Android 8 devices. > > > > Yes, i'd prefer to test it with other Android 8 devices (not sure if it should > > block code-review or not). > > LGTM. I commented into the ticket stating the reasons why I think this is fine. LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
