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)

Product:
Toolkit
Component:
Safe Browsing
Platform:
ARM
Android
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 + verified

People

(Reporter: u549602, Assigned: dlee)

References

Details

(Whiteboard: #sbv4-m10)

Attachments

(1 file)

Bug 1408396 - Fix a deadlock issue when about:url-classifier accesses GetCacheInfo API.
59 bytes, text/x-review-board-request
francois
: review+
Details 
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
Priority: -- → P1
Whiteboard: #sbv4-m10
This sounds pretty bad.

Dimi, would you have time to look into this?
Flags: needinfo?(dlee)
sure
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Flags: needinfo?(dlee)
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)
(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 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+
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)
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
https://hg.mozilla.org/mozilla-central/rev/4c2fb13f193b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.

Attachment

General

Creator:
Created:
Updated:
Size: