Closed Bug 838396 Opened 8 years ago Closed 7 years ago
Not setting has
Mixed Display Content Loaded and has Mixed Display Content Blocked flag in ns Mixed Content Blocker .cpp
We are missing a call to SetHasMixedDisplayContentBlocked in nsMixedContentBlocker.cpp. Missing this shouldn't really cause any issues today, since we aren't doing much with this flag and it doesn't have a UI impact today. But, it is supposed to be set when mixed display content is blocked, so here is a patch that does so.
Attachment #710445 - Flags: review?(bugs)
Comment on attachment 710445 [details] [diff] [review] Missed setting hasMixedDisplayContentBlocked Can we have a test for this (so that we catch possible regressions)-
Attachment #710445 - Flags: review?(bugs) → review+
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b5262e61b9af I need to add a test.
(In reply to Tanvi Vyas [:tanvi] from comment #0) > We are missing a call to SetHasMixedDisplayContentBlocked in > nsMixedContentBlocker.cpp. Missing this shouldn't really cause any issues > today, since we aren't doing much with this flag and it doesn't have a UI > impact today. But, it is supposed to be set when mixed display content is > blocked, so here is a patch that does so. This flag is now used in nsSecureBrowserUIImpl to help calculate the UI state. The easiest way to test this may be to have a test if an <img src=x.png>, where the server returns a 404 response for x.png. In that case, I nsMixedContentBlocker will rightly consider the page to have mixed content, bug the old nsISecureBrowserUIImpl calculations will, I think, wrongly consider the page to be mixed-content free.
Assignee: tanvi → alagenchev
Summary: Not setting hasMixedActiveContentLoaded flag in nsMixedContentBlocker.cpp → Not setting hasMixedDisplayContentLoaded flag in nsMixedContentBlocker.cpp
We forget to set the hasMixedDispalyContentBlocked the following case: * User has explicitly enabled mixed display content blocking in about:config by setting security.mixed_content.block_display_content to true. * User visits a page with mixed display content * The mixed display content load is rejected by nsMixedContetnBlocker.cpp:462, but we never set the hasMixedDisplayContentBlocked flag. We need a rootDoc->SetHasMixedDisplayContentBlocked(true); at line 465. The patch does this (but might not apply cleanly because of change to the file). You can test this by checking the value of hasMixedDisplayContentBlocked, which is exposed in this idl: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#510 Also, this bug has an implication I didn't realize before. We are under-reporting telemetry for pages with mixed display content because we forgot to set this flag. However, the percentage of under-reporting is VERY low since it's only for users who gone to about:config and set the pref to block mixed display. If I had to guess, I would say this would be on the order of 100 users. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#1452
While looking at this more carefully, I realized we also forget to set hasMixedDisplayContentLoaded() in the following case: * User has explicitly enabled mixed display content blocking in about:config by setting security.mixed_content.block_display_content to true. * User visits a page with BOTH mixed active & mixed display content. The user clicks the shield and "disables protection". * The mixed display content load is hence accepted by nsMixedContetnBlocker.cpp:456, but we never set the hasMixedDisplayContentLoaded flag. Fix: We need a rootDoc->SetHasMixedDisplayContentLoaded(true); at line 459. Implications: As in the previous case, there are no UI implications to missing this flag. Thi is because this is a case where there is both mixed display and mixed active content. Hence, the UI would look the same as if there were just mixed active content anyway (yellow triangle). Again, there are telemetry implications where we are under-reporting. But this situation is even more rare than in the previous comment, so I"m not expecting any spikes in the mixed display content telemetry. Test: To test this, we can check the value of hasMixedDisplayContentLoaded after the user "disables protection on this page". This is exposed in this idl: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#503
Summary: Not setting hasMixedDisplayContentLoaded flag in nsMixedContentBlocker.cpp → Not setting hasMixedDisplayContentLoaded and hasMixedDisplayContentBlocked flag in nsMixedContentBlocker.cpp
I am uploading a WIP patch, since I am running into two issues that I can't seem to figure out on my own. The first one is that these checks are failing for some reason after clicking on the doorhanger to allow mixed content: + is(browser.docShell.hasMixedDisplayContentLoaded, true, "hasMixedDisplayContentLoaded flag has not been set"); + is(browser.docShell.hasMixedActiveContentLoaded, true, "hasMixedActiveContentLoaded flag has not been set"); I stepped through the code in nsMixedContentBlocker and the flags are getting set correctly. The checks before clicking on the doorhanger work fine too. The second issue is that when putting the tests in browser/devtools/webconsole/test and running them from there, they run fine. When I try to put them in browser/base/content/test then nothing really happens. I've added both files to the Makefile.in and did mach build. Can I please get some quick pointers about what's going on here? I asked about the first issue on #developers without success. Thanks a lot!
Attachment #795748 - Flags: feedback?(bzbarsky)
So this happens: >+ notification.secondaryActions.callback(); and that starts a new (async) load, right? But the flags are checked immediately, before that load completes. Is that the basic problem? I have no idea what's going on with the test dir issue.
Attachment #795748 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #8) > So this happens: > > >+ notification.secondaryActions.callback(); > > and that starts a new (async) load, right? > > But the flags are checked immediately, before that load completes. Is that > the basic problem? Oh yes! I knew it was something simple I was missing. > I have no idea what's going on with the test dir issue. I'll try to ask around on #developers again. Thanks bz!
Comment on attachment 796334 [details] [diff] [review] 838396.patch r=me
Attachment #796334 - Flags: review?(bzbarsky) → review+
Comment on attachment 796334 [details] [diff] [review] 838396.patch Tanvi asked me to request a review from smaug too, since he's familiar with the MCB code.
Attachment #796334 - Flags: review?(bugs)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.