1321377 - Calling nsUrlClassifierDBService::ClassifyLocalWithTables from the Content process crashes it instead of returning errors

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Robin Steuber (she/her) [:bytesized]]

When an invalid URL is passed to nsUrlClassifierDBService::ClassifyLocalWithTables in the Content processes, the process crashes instead of the function returning an error.

My understanding is that returning |IPC_FAIL(...)| here [1] causes the Content process to crash.

[1] http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/dom/ipc/ContentParent.cpp#4828

Comment 1

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1321377 - When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error

Review commit: https://reviewboard.mozilla.org/r/96626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96626/

Comment 2

[(no longer active)]

Comment on attachment 8815847 [details]
Bug 1321377 - When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error

https://reviewboard.mozilla.org/r/96626/#review96876

Thanks for the fix, looks good to me!

Comment 3

[François Marier [:francois]]

Comment on attachment 8815847 [details]
Bug 1321377 - When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error

https://reviewboard.mozilla.org/r/96626/#review96878

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/835590180676
When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error r=Ehsan,francois

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/835590180676

Comment 6

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8815847 [details]
Bug 1321377 - When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Can't run the study as intended
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for this feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: Bug 1318768, Bug 1323220, Bug 1325255, Bug 1322204, Bug 1325651, Bug 1319571, Bug 1321377, Bug 1307604, Bug 1323064, Bug 1335549, Bug 1333303, Bug 1333483, Bug 1336714, Bug 1338287
[Is the change risky?]: No
[Why is the change risky/not risky?]: Fixes Bug 1318768
[String changes made/needed]: none

Comment 7

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1321377 - When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error (beta uplift)

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8815847 [details]
Bug 1321377 - When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error

It's been in FF53 already. Aurora53-.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8837403 [details] [diff] [review]
Bug 1321377 - When called from the content process, ClassifyLocalWithTables should return failure instead of causing an IPC error (beta uplift)

this was deemed too risky for beta