Closed Bug 850435 Opened 9 years ago Closed 9 years ago

Malware/phishing UI Telmetry does not separately measure iframes

Categories

(Firefox :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: devd, Assigned: devd)

References

Details

Attachments

(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 https://tbpl.mozilla.org/?tree=Try&rev=81c0a8f1190e

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)
https://hg.mozilla.org/mozilla-central/rev/6d3525785f9f
Status: NEW → RESOLVED
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])
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #731593 - Flags: review?(felipc) → review+
Keywords: checkin-needed
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48be75a0907b
Status: REOPENED → RESOLVED
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.