850435 - Malware/phishing UI Telmetry does not separately measure iframes

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Devdatta Akhawe [:devd]]

Bug 767676 added security ui telemetry. If an iframe is marked as malware, it shows a warning in the iframe. A really small iframe would still be counted despite the fact that the user likely never saw the warning. We should instead measure the case for top level warning and iframe level warning separately.

Comment 1

[Devdatta Akhawe [:devd]]

Attachment: Separately measure UI telemetry for iframes and top level

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 730739 [details] [diff] [review]
Separately measure UI telemetry for iframes and top level

Review of attachment 730739 [details] [diff] [review]:
-----------------------------------------------------------------

in nsISecurityUITelemetry, add a comment in the older bucket consts (WARNING_MALWARE_PAGE) saying that they are now unused in favor of the _TOP/_FRAME ones

::: browser/base/content/browser.js
@@ +2745,5 @@
>      let isMalware = /e=malwareBlocked/.test(aOwnerDoc.documentURI);
>      let bucketName = isMalware ? "WARNING_MALWARE_PAGE_":"WARNING_PHISHING_PAGE_";
>      let nsISecTel = Ci.nsISecurityUITelemetry;
> +    let isIframe = (aOwnerDoc.defaultView.parent === aOwnerDoc.defaultView);
> +    bucketName += isIframe ? "TOP_":"FRAME_";

add spaces between :

::: docshell/base/nsDocShell.cpp
@@ +4362,5 @@
> +        bool isIframe = false;
> +        nsCOMPtr<nsIDocShellTreeItem> parentItem;
> +        GetSameTypeParent(getter_AddRefs(parentItem));
> +        if (parentItem)
> +            isIframe = true;

there's a function that already does this :) nsDocShell::IsFrame()

Comment 3

[Devdatta Akhawe [:devd]]

Attachment: Separately measure UI telemetry for iframes and top level

Comment 4

[Devdatta Akhawe [:devd]]

Comment on attachment 730758 [details] [diff] [review]
Separately measure UI telemetry for iframes and top level

carrying over the r+. Try run is at https://tbpl.mozilla.org/?tree=Try&rev=81c0a8f1190e

I will add checkin-needed after the try finishes

Comment 5

[Devdatta Akhawe [:devd]]

Try looks green to me. Asking for checkin.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d3525785f9f

Comment 7

[Devdatta Akhawe [:devd]]

Attachment: Android Patch: separately measure UI telemetry for iframes and top level

Comment 8

[Devdatta Akhawe [:devd]]

Comment on attachment 731593 [details] [diff] [review]
Android Patch: separately measure UI telemetry for iframes and top level

Felipe: Can you please review this too? This is the same 2 line change made to browser.js in the prevoius patch, but now in the Android code. Sorry for missing this earlier---I didn't know there existed a duplicate copy of browser.js functions in the android directory.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d3525785f9f

Comment 10

[Devdatta Akhawe [:devd]]

RyanVM: There is a minor 2 liner that should also ideally go in. (see Attachment #731593 [details] [diff])

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48be75a0907b

I suppose this will need Aurora uplift approval then too. In the future, you can put [leave open] on the whiteboard to keep the bug from being resolved when merged to m-c.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48be75a0907b