Closed Bug 850435 Opened 9 years ago Closed 9 years ago
Malware/phishing UI Telmetry does not separately measure iframes
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 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.
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)
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+
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 22 → Firefox 23
You need to log in before you can comment on or make changes to this bug.