Closed Bug 1504728 Opened 5 years ago Closed 5 years ago

Add a content blocking notification for indicating when a top-level page is using cookies/site data

Categories

(Firefox :: Protections UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

We need to have a new notification for indicating whether a new top-level page is using cookies/site data.  It doesn't matter whether the said data is being allowed/blocked.
Priority: -- → P2
Status: NEW → ASSIGNED
Priority: P2 → P1
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/365e0d2414bf
Add a content blocking notification for indicating when a top-level page is using cookies or site data r=baku,valentin
This patch breaks this test, among others:

https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/browser/base/content/test/siteIdentity/browser_mixed_content_cert_override.js#40

I think this is hitting an unrelated bug in the secure UI code.  What is happening is that I'm adding more SecurityChange notifications that are now being dispatched as a result of cookies being present.  This results in nsSecureBrowserUIImpl::GetState() being called more frequently than before.

Now we hit this tricky case, where we run CheckForBlockedContent() several times.  If I set MOZ_LOG to nsSecureBrowserUI:5 I get a log looking like this right before the test failure:

 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI GetState 0x7fd619ad42e0
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI   mState: 4088002
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI GetState 0x7fd619ad42e0
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI   mState: 4088002
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI GetState 0x7fd619ad42e0
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI   mState: 4008201
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI GetState 0x7fd619ad42e0
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI   mState: 4008201
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI GetState 0x7fd619ad42e0
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI   mState: 4008201
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI GetState 0x7fd619ad42e0
 0:15.43 GECKO(14973) [Child 15360: Main Thread]: D/nsSecureBrowserUI   mState: 4008201

Note how we're starting with STATE_IS_SECURE and then we *lose* that flag as well as STATE_SECURE_HIGH and gain STATE_IS_BROKEN and STATE_LOADED_MIXED_DISPLAY_CONTENT.  This means we take this branch: <https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/security/manager/ssl/nsSecureBrowserUIImpl.cpp#165>.  Now, this also means that the next time around when GetState() is called, since STATE_IS_SECURE isn't set, we can never take this branch <https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/security/manager/ssl/nsSecureBrowserUIImpl.cpp#158> so even though |docShell->GetHasMixedActiveContentLoaded()| would return true, we'd never set STATE_LOADED_MIXED_ACTIVE_CONTENT.

So in short, if something causes extra SecurityChange notifications to be dispatched, depending on the order of events we may set the mixed passive content flag and never be able to set the mixed passive flag, or vice versa for that matter.

I *think* the right fix here is to remove the |(mState & STATE_IS_SECURE)| check on line 158 here and move all of the nesting four checks to happen unconditionally, but I'd like a second pair of eyes to check through my logic above to make sure I'm not missing something about why this code works the way it does.  Dana, looks like you're familiar with this code, do you mind having a look at this comment please and let me know what you think?  Thanks!
Flags: needinfo?(ehsan) → needinfo?(dkeeler)
Yes, this looks like a preexisting bug. If I recall correctly, though, unconditionally checking for the various flavors of mixed content can result in showing the lock-with-yellow-triangle icon on http:// pages (e.g. if we have http -> https iframe -> http frame like http://mixed-content-tests-mozilla.org/tvyas/mixedgrandiframe.html ), which I think is not what we want. It looks like what we could do is expand that check to be `(mState & STATE_IS_SECURE) || (mState & STATE_IS_BROKEN)` since STATE_IS_BROKEN should only ever happen with pages that were STATE_IS_SECURE.

(also just fyi STATE_IS_SECURE_HIGH is a red herring - see bug 1095602)
Flags: needinfo?(dkeeler)
(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #5)
> Yes, this looks like a preexisting bug. If I recall correctly, though,
> unconditionally checking for the various flavors of mixed content can result
> in showing the lock-with-yellow-triangle icon on http:// pages (e.g. if we
> have http -> https iframe -> http frame like
> http://mixed-content-tests-mozilla.org/tvyas/mixedgrandiframe.html ), which
> I think is not what we want.

Interesting.  FWIW removing the |(mState & STATE_IS_SECURE)| check doesn't seem to change our security UI on that page for some reason...

> It looks like what we could do is expand that
> check to be `(mState & STATE_IS_SECURE) || (mState & STATE_IS_BROKEN)` since
> STATE_IS_BROKEN should only ever happen with pages that were STATE_IS_SECURE.

Great, that is definitely a sufficient fix for the issue I'm seeing here.  Patch is coming up...
Keywords: leave-open
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6eaed759f7d
Part 0: Update nsSecureBrowserUIImpl::CheckForBlockedContent() to check for mixed content blocking when the resource is already marked as broken; r=keeler
Attachment #9023113 - Attachment description: Bug 1504728 - Add a content blocking notification for indicating when a top-level page is using cookies or site data → Bug 1504728 - Part 1: Add a content blocking notification for indicating when a top-level page is using cookies or site data
Depends on: 1507352
Depends on: 1507353
Keywords: leave-open
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc39602006be
Part 1: Add a content blocking notification for indicating when a top-level page is using cookies or site data r=baku,valentin,johannh
Depends on: 1507689
Depends on: 1508114
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfc775a144ad
Part 1: Add a content blocking notification for indicating when a top-level page is using cookies or site data r=baku,valentin,johannh
Flags: needinfo?(ehsan)
Backed out changeset dfc775a144ad (bug 1504728) for Brwoser-chrome failure in browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=212631099&repo=autoland&lineNumber=2390

 INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Trackers view was shown - 
[task 2018-11-19T16:05:31.432Z] 16:05:31     INFO - Buffered messages finished
[task 2018-11-19T16:05:31.433Z] 16:05:31     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | We have 2 trackers in the list - Got 1, expected 2
[task 2018-11-19T16:05:31.434Z] 16:05:31     INFO - Stack trace:
[task 2018-11-19T16:05:31.435Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:test_is:1303
[task 2018-11-19T16:05:31.436Z] 16:05:31     INFO - chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:assertSitesListed/<:86
[task 2018-11-19T16:05:31.437Z] 16:05:31     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:111
[task 2018-11-19T16:05:31.437Z] 16:05:31     INFO - chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:assertSitesListed:43
[task 2018-11-19T16:05:31.438Z] 16:05:31     INFO - chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:testTrackersSubView:104
[task 2018-11-19T16:05:31.439Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-11-19T16:05:31.440Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-11-19T16:05:31.441Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:982
[task 2018-11-19T16:05:31.442Z] 16:05:31     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2018-11-19T16:05:31.443Z] 16:05:31     INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Has an item for trackertest.org - 
[task 2018-11-19T16:05:31.444Z] 16:05:31     INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | List item is visible - 
[task 2018-11-19T16:05:31.445Z] 16:05:31     INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Indicates whether the tracker was blocked or allowed - 
[task 2018-11-19T16:05:31.446Z] 16:05:31     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-11-19T16:05:31.447Z] 16:05:31     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Has an item for itisatracker.org - 
[task 2018-11-19T16:05:31.448Z] 16:05:31     INFO - Stack trace:
[task 2018-11-19T16:05:31.449Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:test_ok:1292
[task 2018-11-19T16:05:31.450Z] 16:05:31     INFO - chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:assertSitesListed/<:95
[task 2018-11-19T16:05:31.451Z] 16:05:31     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:111
[task 2018-11-19T16:05:31.451Z] 16:05:31     INFO - chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:assertSitesListed:43
[task 2018-11-19T16:05:31.452Z] 16:05:31     INFO - chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:testTrackersSubView:104
[task 2018-11-19T16:05:31.453Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-11-19T16:05:31.454Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-11-19T16:05:31.455Z] 16:05:31     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:982
[task 2018-11-19T16:05:31.456Z] 16:05:31     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2018-11-19T16:05:31.457Z] 16:05:31     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-11-19T16:05:31.458Z] 16:05:31     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Uncaught exception - at resource://testing-common/BrowserTestUtils.jsm:257 - TypeError: element is undefined; can't access its "ownerGlobal" property
[task 2018-11-19T16:05:31.459Z] 16:05:31     INFO - Stack trace:
[task 2018-11-19T16:05:31.459Z] 16:05:31     INFO - is_visible@resource://testing-common/BrowserTestUtils.jsm:257:9
[task 2018-11-19T16:05:31.460Z] 16:05:31     INFO - assertSitesListed/<@chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:96:8
[task 2018-11-19T16:05:31.461Z] 16:05:31     INFO - async*withNewTab@resource://testing-common/BrowserTestUtils.jsm:111:24
[task 2018-11-19T16:05:31.462Z] 16:05:31     INFO - async*assertSitesListed@chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:43:9
[task 2018-11-19T16:05:31.463Z] 16:05:31     INFO - async*testTrackersSubView@chrome://mochitests/content/browser/browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js:104:9
[task 2018-11-19T16:05:31.463Z] 16:05:31     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1093:34
[task 2018-11-19T16:05:31.464Z] 16:05:31     INFO - async*Tester_execTest@chrome://mochikit/content/browser-test.js:1084:16
[task 2018-11-19T16:05:31.465Z] 16:05:31     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:982:9
[task 2018-11-19T16:05:31.466Z] 16:05:31     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
[task 2018-11-19T16:05:31.467Z] 16:05:31     INFO - Leaving test bound testTrackersSubView
[task 2018-11-19T16:05:31.468Z] 16:05:31     INFO - Entering test bound cleanup
[task 2018-11-19T16:05:31.469Z] 16:05:31     INFO - Leaving test bound cleanup
[task 2018-11-19T16:05:31.470Z] 16:05:31     INFO - GECKO(2844) | MEMORY STAT | vsize 720MB | residentFast 323MB | heapAllocated 118MB
[task 2018-11-19T16:05:31.471Z] 16:05:31     INFO - TEST-OK | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | took 1239ms
[task 2018-11-19T16:05:31.471Z] 16:05:31     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-11-19T16:05:31.472Z] 16:05:31     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Found an unexpected tab at the end of test run: http://tracking.example.org/browser/browser/base/content/test/trackingUI/trackingPage.html - 
[task 2018-11-19T16:05:31.473Z] 16:05:31     INFO - checking window state

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dfc775a144ad2294fb2bb1f82b24ca136819d39b

Backout:
https://hg.mozilla.org/integration/autoland/rev/e4658af66fdf23ad02f225a2f808bd346127c700
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94efd169438e
Part 1: Add a content blocking notification for indicating when a top-level page is using cookies or site data r=baku,valentin,johannh
Backed out for failing mochitest AddressSanitizer 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=212746635&revision=94efd169438e3892c8d226ab762f54cb15d82caf

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=212733437&repo=autoland&lineNumber=1249

22:52:28     INFO - TEST-START | dom/media/mediasource/test/test_MediaSource_memory_reporting.html
22:52:32     INFO - GECKO(2552) | =================================================================
22:52:32    ERROR - GECKO(2552) | ==1824==ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x129707103eb8
22:52:32     INFO - GECKO(2552) | ==1824==WARNING: Failed to use and restart external symbolizer!
22:52:33     INFO - GECKO(2552) |     #0 0x7ff96343f1c6 in __asan::asan_malloc_usable_size Z:\task_1541608776\build\src\build\build-clang\build-clang\src\llvm\projects\compiler-rt\lib\asan\asan_allocator.cc:959
22:52:33     INFO - GECKO(2552) |     #1 0x7ff944fef2de in mozilla::dom::ContentBlockingLog::AddSizeOfExcludingThis z:\build\build\src\obj-firefox\dist\include\mozilla\dom\ContentBlockingLog.h:168
22:52:33     INFO - GECKO(2552) |     #2 0x7ff944ff0298 in nsDocument::DocAddSizeOfExcludingThis z:\build\build\src\dom\base\nsDocument.cpp:12142
22:52:33     INFO - GECKO(2552) |     #3 0x7ff944bededc in nsGlobalWindowInner::AddSizeOfIncludingThis z:\build\build\src\dom\base\nsGlobalWindowInner.cpp:6856
22:52:33     INFO - GECKO(2552) |     #4 0x7ff945172fe6 in CollectWindowReports z:\build\build\src\dom\base\nsWindowMemoryReporter.cpp:322
22:52:33     INFO - GECKO(2552) |     #5 0x7ff94516afaa in nsWindowMemoryReporter::CollectReports z:\build\build\src\dom\base\nsWindowMemoryReporter.cpp:582
22:52:33     INFO - GECKO(2552) |     #6 0x7ff94124b73f in mozilla::detail::RunnableFunction<`lambda at z:/build/build/src/xpcom/base/nsMemoryReporterManager.cpp:1973:5'>::Run z:\build\build\src\obj-firefox\dist\include\nsThreadUtils.h:577
22:52:33     INFO - GECKO(2552) |     #7 0x7ff9413c3763 in nsThread::ProcessNextEvent z:\build\build\src\xpcom\threads\nsThread.cpp:1244
22:52:33     INFO - GECKO(2552) |     #8 0x7ff9413cc038 in NS_ProcessNextEvent z:\build\build\src\xpcom\threads\nsThreadUtils.cpp:530
22:52:33     INFO - GECKO(2552) |     #9 0x7ff94247ce79 in mozilla::ipc::MessagePump::Run z:\build\build\src\ipc\glue\MessagePump.cpp:97
22:52:33     INFO - GECKO(2552) |     #10 0x7ff9423e001e in MessageLoop::RunHandler z:\build\build\src\ipc\chromium\src\base\message_loop.cc:318
22:52:33     INFO - GECKO(2552) |     #11 0x7ff9423dfda6 in MessageLoop::Run z:\build\build\src\ipc\chromium\src\base\message_loop.cc:298
22:52:33     INFO - GECKO(2552) |     #12 0x7ff94b1e54da in nsBaseAppShell::Run z:\build\build\src\widget\nsBaseAppShell.cpp:158
22:52:33     INFO - GECKO(2552) |     #13 0x7ff94b375c37 in nsAppShell::Run z:\build\build\src\widget\windows\nsAppShell.cpp:420
22:52:33     INFO - GECKO(2552) |     #14 0x7ff94f97393d in XRE_RunAppShell z:\build\build\src\toolkit\xre\nsEmbedFunctions.cpp:961
22:52:33     INFO - GECKO(2552) |     #15 0x7ff9423e001e in MessageLoop::RunHandler z:\build\build\src\ipc\chromium\src\base\message_loop.cc:318
22:52:33     INFO - GECKO(2552) |     #16 0x7ff9423dfda6 in MessageLoop::Run z:\build\build\src\ipc\chromium\src\base\message_loop.cc:298
22:52:33     INFO - GECKO(2552) |     #17 0x7ff94f972b52 in XRE_InitChildProcess z:\build\build\src\toolkit\xre\nsEmbedFunctions.cpp:787
22:52:33     INFO - GECKO(2552) |     #18 0x7ff7d4cb2038 in Ordinal0+0x2038 (Z:\task_1542667515\build\application\firefox\firefox.exe+0x140002038)
22:52:33     INFO - GECKO(2552) |     #19 0x7ff7d4cb14a1 in Ordinal0+0x14a1 (Z:\task_1542667515\build\application\firefox\firefox.exe+0x1400014a1)
22:52:33     INFO - GECKO(2552) |     #20 0x7ff7d4d8954b in TargetNtUnmapViewOfSection+0x28cdb (Z:\task_1542667515\build\application\firefox\firefox.exe+0x1400d954b)
22:52:33     INFO - GECKO(2552) |     #21 0x7ff983952773 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180012773)
22:52:33     INFO - GECKO(2552) |     #22 0x7ff983bb0d60 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x180070d60)
22:52:33     INFO - GECKO(2552) | 0x129707103eb8 is located 8 bytes inside of 16-byte region [0x129707103eb0,0x129707103ec0)



Backout: https://hg.mozilla.org/integration/autoland/rev/104d7f34de53a9359d6a1983b8d421a60a649592
Depends on: 1508527
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/209ef31926be
Part 1: Add a content blocking notification for indicating when a top-level page is using cookies or site data r=baku,valentin,johannh
https://hg.mozilla.org/mozilla-central/rev/209ef31926be
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Depends on: 1508944
Depends on: 1509340
Depends on: 1510275
No longer blocks: 1509161
Depends on: 1509161
No longer depends on: 1510275
You need to log in before you can comment on or make changes to this bug.