Closed Bug 850435 Opened 9 years ago Closed 9 years ago

Malware/phishing UI Telmetry does not separately measure iframes


(Firefox :: Security, defect)

Not set



Firefox 23


(Reporter: devd, Assigned: devd)




(2 files, 1 obsolete file)

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.
Assignee: nobody → dev.akhawe+mozilla
Attachment #730739 - Flags: review?(felipc)
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()
Attachment #730739 - Flags: review?(felipc) → review+
Attachment #730739 - Attachment is obsolete: true
Comment on attachment 730758 [details] [diff] [review]
Separately measure UI telemetry for iframes and top level

carrying over the r+. Try run is at

I will add checkin-needed after the try finishes
Attachment #730758 - Flags: review+
Try looks green to me. Asking for checkin.
Keywords: checkin-needed
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.
Attachment #731593 - Flags: review?(felipc)
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
RyanVM: There is a minor 2 liner that should also ideally go in. (see Attachment #731593 [details] [diff])
Resolution: FIXED → ---
Attachment #731593 - Flags: review?(felipc) → review+
Keywords: checkin-needed

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.
Keywords: checkin-needed
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 22 → Firefox 23
Blocks: 878606
No longer blocks: 878606
Depends on: 901821
You need to log in before you can comment on or make changes to this bug.