Closed
Bug 1408396
Opened 7 years ago
Closed 7 years ago
Failing Updating Safebrowser DB will trigger a frozen browser
Categories
(Toolkit :: Safe Browsing, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox56 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | + | verified |
People
(Reporter: u549602, Assigned: dimi)
References
Details
(Whiteboard: #sbv4-m10)
Attachments
(1 file)
Build : 58.0a1 (2017-10-13) Device: Pixel C Android 7.1.1 Steps To Reproduce: 1. Go to about:url-classifier page 2. Press Trigger Update for google 4 3. Disconnect your Wi-Fi when the status is ""updating"" 4. Reconnect your Wi-Fi 5. Tap on Trigger Update 6. Restart the browser open about:url-classifier again 7. Press ""Trigger Update"" again" Expected: Last update status should be ""success"". Safebrowsing v4 DB is updated" Actual : Step 5.The status becomes "cannot update" Step 7.The browser freezes and it will still be frozen even after its restart or device restart For further details please check: https://www.youtube.com/watch?v=4cwZCBrHMDc&feature=youtu.be
This also occurs by following : 1. Go to about:url-classifier page 2. Press ""Trigger Update"" for google 4 3. Close Firefox browser when the status is ""updating"" 4. Re-open Firefox browser Please check : https://www.youtube.com/watch?v=qAIyAp9oF2g&feature=youtu.be for further details
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: #sbv4-m10
Comment 2•7 years ago
|
||
This sounds pretty bad. Dimi, would you have time to look into this?
Flags: needinfo?(dlee)
Assignee | ||
Comment 3•7 years ago
|
||
sure
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Flags: needinfo?(dlee)
Assignee | ||
Comment 4•7 years ago
|
||
The "cannot update" message in Comment 1 Step 5 is correct behavior because of SafeBrowsing backoff mechanism. As for the freeze issue, I found this is not related to close firefox during an update, I can simply reproduce this bug by just updating V4 with about:url-classifier and reopen firefox. The root cause is that there is a deadlock when main thread(about:url-classifier page) ask to get cache information via non-main thread[1] and then non-main thread want to initialize NSS component via main thread[2]. I'll fix this by changing |GetCacheInfo| info API to asynchronous tomorrow. [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp#255 [1] https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSComponent.cpp#103
Comment 5•7 years ago
|
||
Tracking for 56, sounds like something we should fix before launching this feature in 58.
tracking-firefox58:
--- → +
Comment 6•7 years ago
|
||
(In reply to Dimi Lee[:dimi][:dlee] from comment #4) > The root cause is that there is a deadlock when main > thread(about:url-classifier page) ask to get cache information via non-main > thread[1] and then non-main thread want to initialize NSS component via main > thread[2]. Is that deadlock restricted to about:url-classifier or could it happen outside of that debug page?
Flags: needinfo?(dlee)
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to François Marier [:francois] from comment #6) > > Is that deadlock restricted to about:url-classifier or could it happen > outside of that debug page? Only about:url-classifier may cause this problem since it is the only one uses that sync API (That API is designed for about page). BTW desktop url-classifier won't have this problem because NSS module is already initialized before about:url-classifier calls |GetCacheInfo|.
Flags: needinfo?(dlee)
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8919206 [details] Bug 1408396 - Fix a deadlock issue when about:url-classifier accesses GetCacheInfo API. https://reviewboard.mozilla.org/r/190128/#review195618 ::: toolkit/components/url-classifier/nsUrlClassifierProxies.cpp:255 (Diff revision 1) > - } > +} > > - // This blocks main thread but since 'GetCacheInfo' is only used by > - // about:url-classifier so it should be fine. > - mozilla::SyncRunnable::DispatchToThread(t, r); > - return NS_OK; > +NS_IMETHODIMP > +UrlClassifierDBServiceWorkerProxy::GetCacheInfoRunnable::Run() > +{ > + mTarget->GetCacheInfo(mTable, &mCache); Is it worth adding a `MOZ_ASSERT(mCallback)` here?
Attachment #8919206 -
Flags: review?(francois) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d5805d3113c43968985b6cb5dbd0830b300b7e
Comment 12•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s bce8eb181ff3 -d 92a0d2f17a62: rebasing 428039:bce8eb181ff3 "Bug 1408396 - Fix a deadlock issue when about:url-classifier accesses GetCacheInfo API. r=francois" (tip) merging toolkit/content/aboutUrlClassifier.js warning: conflicts while merging toolkit/content/aboutUrlClassifier.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by dlee@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c2fb13f193b Fix a deadlock issue when about:url-classifier accesses GetCacheInfo API. r=francois
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c2fb13f193b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Reporter | ||
Comment 16•7 years ago
|
||
Verified as fixed on the latest Nightly build 58.0a1(2017-10-27). Issue tested on : Samsung galaxy S6 EDGE - Android 7.0 Pixel C - Android 8.0 Xiaomi mi i4 - Android 5.0.2
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•