Failing Updating Safebrowser DB will trigger a frozen browser

VERIFIED FIXED in Firefox 58

Status

()

Toolkit
Safe Browsing
P1
normal
VERIFIED FIXED
a month ago
28 days ago

People

(Reporter: Ninu, Assigned: dimi)

Tracking

(Blocks: 1 bug)

Trunk
mozilla58
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected, firefox58+ verified)

Details

(Whiteboard: #sbv4-m10)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
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
(Reporter)

Comment 1

a month ago
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
Priority: -- → P1
Whiteboard: #sbv4-m10
This sounds pretty bad.

Dimi, would you have time to look into this?
Flags: needinfo?(dlee)
(Assignee)

Comment 3

a month ago
sure
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Flags: needinfo?(dlee)
(Assignee)

Comment 4

a month 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
Tracking for 56, sounds like something we should fix before launching this feature in 58.
tracking-firefox58: --- → +
(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

a month 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

a month 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

a month ago
try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d5805d3113c43968985b6cb5dbd0830b300b7e

Comment 12

a month 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

a month 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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c2fb13f193b
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Reporter)

Comment 16

28 days 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
status-firefox58: fixed → verified
You need to log in before you can comment on or make changes to this bug.