Opened 14 months ago

Closed 10 months ago

Last modified 10 months ago

#4516 closed defect (fixed)

[HA Crash] iOS10 NSFetchRequest fails while servicing chrome.storage.set

Reported by: pavelz Assignee:
Priority: P1 Milestone: Adblock-Browser-for-iOS-1.5.2
Module: Adblock-Browser-for-iOS Keywords:
Cc: mario, jand, fhd Blocked By: #4672
Blocking: Platform: Adblock Browser for iOS
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by pavelz)

Environment

*iOS10*
ABB 1.5.1 Appstore release

How to reproduce

Unknown. Possibly an unfortunate timing of filter list update

Observed behaviour

Crash which may be completely sudden, as it is induced by usage of chrome.storage.local.set in the background script

MOST FREQUENT, all iOS10 versions
https://rink.hockeyapp.net/manage/apps/310687/app_versions/27/crash_reasons/139881718

1/10th of the first one due to happening just on 10.0.2 which is being upgraded gradually to 10.1
https://rink.hockeyapp.net/manage/apps/310687/app_versions/27/crash_reasons/139963337

Expected behaviour

No crash

Change History (11)

comment:1 Changed 14 months ago by pavelz

Follow up on the same issue in 1.5.1 release
https://issues.adblockplus.org/ticket/4435

comment:3 Changed 13 months ago by pavelz

  • Description modified (diff)

comment:4 Changed 13 months ago by pavelz

comment:5 Changed 13 months ago by pavelz

Containment strategies:

  1. (Eyeo) sift the user feedback for some common objection
  2. (Eyeo) test filterlist enabling/disabling AND BACKGROUND UPDATING (todo: how to induce that>?)
  3. (Salsita) review the core code for all CoreData access, assuming and understanding the calling environment
  4. (Salsita) instrument CoreData access points with main thread assertion, and run intensively
  5. (Salsita) consider replacing CoreData with something simpler and more appropriate to the task (Realm.io or even plain NSUserDefaults)

comment:6 Changed 13 months ago by pavelz

Containment update:

  1. didn't turn out anything
  2. no success yet
  3. 4. reviewed and instrumented by @jand, no catch

We are proposing 5, and that in way of dropping usage of CoreData (which is SQLite beneath) for storage keys file:*. The storage content will be saved in file(s) with the actual name. This was the original usage pattern and the long megabytes of filter list content seem to be not the right pattern for SQLite.

Mind that this is all a speculative change, as we don't know what's the real problem for sure, until a reproduction scenario is found. But simplifying things is rarely a wrong thing to do.

comment:7 Changed 13 months ago by pavelz

  • Cc fhd added

It was found out that chrome.storage.set with key file:patterns.ini is called TWICE per a filter list update, with a slight delay of 1-2 seconds. This is reproducible on desktop Chrome. So the latest speculative crash scenario is an autoupdate which fires right in the middle of loading a heavy webpage. Then two threads are competing intensively and concurrently over allocation in one app's memory pool: app main thread on behalf of CoreData and webthread on behalf of WebKit. It would explain why the bug is so hard to reproduce. Not even an UI button to force subscription update (a la options page) would simulate - a web page needs to be loading at the same moment.

So yet another possible mitigation arises: defer CoreData storing if a page load is underway at the moment.

Preventing ABP extension from storing those 5-10MB twice would be great as well, of course.

comment:8 Changed 13 months ago by fhd

Yup, I can verify that chrome.storage.local.set is being called twice when a filter list update is forced in ABP for Chrome on desktop.

The two write operations are triggered at about the same time, by a subscription.updated and subscription.downloadStatus event respectively (see https://hg.adblockplus.org/adblockpluscore/file/tip/lib/filterListener.js). The first write happens immediately, while the second write is being queued, and happens after the first one finishes.

But there should be just one write in the first place. I'll investigate this, but in the mean time, here's an idea for a workaround: You could block for a little after each write, maybe that could avoid crashing when processing queued writes.

comment:9 Changed 11 months ago by pavelz

  • Blocked By 4672 added

comment:10 Changed 10 months ago by pavelz

  • Resolution set to fixed
  • Status changed from new to closed

comment:11 Changed 10 months ago by mario

  • Milestone set to Adblock-Browser-for-iOS-1.5.2
Note: See TracTickets for help on using tickets.