Closed Bug 1321377 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- affected
firefox53 --- fixed

People

(Reporter: bytesized, Assigned: bytesized)

References

Details

Attachments

(2 files)

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
Attachment #8815847 - Flags: review?(francois)
Attachment #8815847 - Flags: review?(ehsan)
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!
Attachment #8815847 - Flags: review?(ehsan) → review+
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
Attachment #8815847 - Flags: review?(francois) → review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/835590180676
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8815847 - Flags: approval-mozilla-aurora?
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-.
Attachment #8815847 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Attachment #8837403 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.