Closed Bug 1504728 Opened 6 years ago Closed 6 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)

Product:
Firefox
Component:
Protections UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

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

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1504728 - Part 1: Add a content blocking notification for indicating when a top-level page is using cookies or site data
47 bytes, text/x-phabricator-request
Details | Review
Bug 1504728 - Part 0: Update nsSecureBrowserUIImpl::CheckForBlockedContent() to check for mixed content blocking when the resource is already marked as broken
47 bytes, text/x-phabricator-request
Details | Review
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: 6 years ago
status-firefox65: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: