Closed Bug 1229177 Opened 4 years ago Closed 4 years ago

[e10s] Security UI is not updated when TP or SB block XHR / Fetch requests

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
e10s m8+ ---
firefox45 --- fixed
firefox46 --- verified

People

(Reporter: francois, Assigned: mrbkap)

References

Details

Attachments

(3 files, 2 obsolete files)

The Security UI (TP shield, devtool console message) is updated when a resource is blocked in necko for TP reasons. That's currently done by the SetBlockedTrackingContent() function:

https://dxr.mozilla.org/mozilla-central/rev/45273bbed8efaface6f5ec56d984cb9faf4fbb6a/netwerk/base/nsChannelClassifier.cpp#471

and in a normal e10s subresource load, that function is called twice (for the same channel URL) in two different nsIChannels (the parent and the child).

However, in the case of Fetch/XHR loads, that function is only called once (in the parentChannel) so we can't update the UI. Without e10s, that function is called once in the child so everything works fine.
Assignee: nobody → mrbkap
Attached file xhr.html
Steps to reproduce:

1. put both xhr.html and tracker.js in the local web server root

2. serve these files with a CORS header:

    <Directory /var/www/tp>
      Header set Access-Control-Allow-Origin: *
    </Directory>

3. redirect the hard-coded tracking domains to localhost through /etc/hosts file:

    127.0.0.1    localhost itisatracker.org trackertest.org

4. add these domains as aliases in the web server config

    <VirtualHost localhost:80>
      ServerName localhost
      ServerAlias itisatracker.org
      ServerAlias trackertest.org
    </VirtualHost>

5. visit http://localhost/xhr.html under both e10s and non-e10s
Attached file tracker.js
Expected:

In both e10s and non-e10s, we should see the TP shield in the URL bar and a devtool console warning.

Actual:

The shield and console message only work on non-e10s.
Attachment #8694409 - Attachment description: Test page → xhr.html
Duplicate of this bug: 1232487
Bug 1232487 is another example of this problem, this time with assets loaded within Flash movies.
Attached patch browser test (obsolete) — Splinter Review
This test shows the symptoms described in comment 0. It's possible to actually write a test that we show the notification in the URL bar but I haven't written that part of the test yet. I'm going to use this to debug what's going on here. Now that I have this, it should be pretty simple to debug and fix this bug.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #5)
> This test shows the symptoms described in comment 0.

Naming nit: the shield is no longer used for mixedcontent, it's only for tracking protection now.

Also, if you want another quick way to test this, you can use the test case on https://bugzilla.mozilla.org/show_bug.cgi?id=1217236#c1
Attached patch patch v1 (obsolete) — Splinter Review
The comment in HttpChannelChild.cpp says it all. The CORs listener is stomping on our Cancel() value so we don't update the UI. This is a bit of a hack to recover the original reason the channel was canceled.
Attachment #8700255 - Flags: review?(jduell.mcbugs)
Attachment #8699806 - Attachment is obsolete: true
Comment on attachment 8700255 [details] [diff] [review]
patch v1

Review of attachment 8700255 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_trackingUI_6.js
@@ +5,5 @@
> +    let n = 0;
> +    let listener = {
> +      onSecurityChange: function() {
> +        n = n + 1;
> +        info ("Recieved onSecurityChange event " + n + " of " + numChanges);

i before e, except after c :)
Attachment #8700255 - Flags: review?(jduell.mcbugs) → review+
Attachment #8700255 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/74d2f955790a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
I tested with e10s both on and off, using test cases for fetch, XHR and Flash.

Confirmed issue in Fx43.0.4.
Verified fixed in Fx46 Nightly, 2016-01-18.
Status: RESOLVED → VERIFIED
Blake should we uplift this to 45?
Flags: needinfo?(mrbkap)
Comment on attachment 8700778 [details] [diff] [review]
Patch for checkin

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: The user would not be notified about blocked content.
[Describe test coverage new/current, TreeHerder]: I added a test for this case.
[Risks and why]: Low risk, code touched only affects this specific case (NS_ERROR_TRACKING_URI).
[String/UUID change made/needed]: n/a.
Flags: needinfo?(mrbkap)
Attachment #8700778 - Flags: approval-mozilla-aurora?
Comment on attachment 8700778 [details] [diff] [review]
Patch for checkin

has tests, fix a problem for the e10s experiment, taking it.
Attachment #8700778 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.