Closed
Bug 1044181
Opened 10 years ago
Closed 10 years ago
All loaders that create channels with LOAD_CLASSIFY_URI need to annotate the DOM with blocked nodes
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: mmc, Assigned: geko1702+bugzilla)
References
Details
Attachments
(3 files, 16 obsolete files)
(deleted),
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Only the script and image and iframe loaders are doing this right now. Similar to bug 1029887.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → gkontaxis
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8462930 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467432 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467449 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467452 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8467453 [details] [diff] [review]
object loader now annotates classified tracking nodes
Similar and complementary to bug 1029887
https://tbpl.mozilla.org/?tree=Try&rev=a70a684de762
Attachment #8467453 -
Flags: review?(khuey)
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467450 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to gkontaxis from comment #8)
> Created attachment 8467458 [details] [diff] [review]
> style loader now annotates classified tracking nodes
Notice the existingData patch. The loader has logic to piggyback on existing load requests for the same URI but this has two side effects: 1) it may piggyback on a prefetching request (for the same URI) 2) it may replace an aLoadData object carrying a valid mOwningElement with an existingData object carrying a null mOwningElement. This results in a null mOwningElement when all bundled requests reach OnStreamComplete(). We require a valid mOwingElement (DOM node) to annotate it as a tracking node. Therefore this patch piggybacks on an existingData object only if it has a valid mOwningElement.
https://tbpl.mozilla.org/?tree=Try&rev=d250af5e83ec
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467458 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to gkontaxis from comment #9)
> (In reply to gkontaxis from comment #8)
> > Created attachment 8467458 [details] [diff] [review]
> > style loader now annotates classified tracking nodes
>
> Notice the existingData patch. The loader has logic to piggyback on existing
> load requests for the same URI but this has two side effects: 1) it may
> piggyback on a prefetching request (for the same URI) 2) it may replace an
> aLoadData object carrying a valid mOwningElement with an existingData object
> carrying a null mOwningElement. This results in a null mOwningElement when
> all bundled requests reach OnStreamComplete(). We require a valid
> mOwingElement (DOM node) to annotate it as a tracking node. Therefore this
> patch piggybacks on an existingData object only if it has a valid
> mOwningElement.
>
> https://tbpl.mozilla.org/?tree=Try&rev=d250af5e83ec
Scratch that. What actually happens is that the piggybacking creates a linked list of all sheet load instances depending on a single request. When OnStreamComplete() fires one must process that linked list to locate all sheet load instances that have been served.
It may be the case the first instance in the list does not have an owning element (e.g., case of a prefetching load instance) but the second one does because it originates from an actual link element in the DOM and as such has a valid owning element we can annotate.
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467925 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467977 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8467981 [details] [diff] [review]
style loader now annotates classified tracking nodes
Once the loader finishes processing a request (which could actually be tied to multiple sheet loads) we check its status and if it's NS_ERROR_TRACKING_URI then that means the request was cancelled because the URI is a tracking URI. We want to store weak references to the link nodes that initiated the respective sheet loads. Ultimately a UI will let the user know which and how many nodes have been blocked for this reason.
Attachment #8467981 -
Flags: review?(dbaron)
Attachment #8467453 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Could you explain what this is needed for? I'm trying to understand whether the patch's handling of various null-pointers is appropriate, and that requires understanding what it's for.
I'm worried about two things:
(1) null doc - I *think* this will only happen if a load completes at some point during teardown of the document, but I'm not 100% sure of that
(2) null data->mOwningElement - this can happen for @imported sheets (child sheets) and for sheets loaded through some internal APIs. What does calling doc->AddBlockedTrackingNode(nullptr) do?
Stylistic nits:
* no "this->"
* space before ( in "while(data" -- although I'd actually suggest using a for() instead.
Flags: needinfo?(gkontaxis)
Assignee | ||
Comment 18•10 years ago
|
||
We have back-end logic in place that cancels the channel for URIs that are part of a tracking blacklist we maintain. Then, when the respective elements fail to load we want to add weak references to them in a special "blocked elements" array under their respective document. This is useful for when the UI wants to provide the user with information about what has been blocked.
So for style links that fail to load because their URLs were found in the blacklist we want to do this annotation here.
(1) I'm covering my basis.
(2) Yes. This includes prefetching. I observed that usually the prefetching sheet load was bundled with the actual load so that the first mOwningElement was null and the second (in the linked list) was the actual link node. AddBlockedTrackingNode() handles null pointers just fine (checks and returns immediately)
Flags: needinfo?(gkontaxis)
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to gkontaxis from comment #18)
> We have back-end logic in place that cancels the channel for URIs that are
> part of a tracking blacklist we maintain. Then, when the respective elements
> fail to load we want to add weak references to them in a special "blocked
> elements" array under their respective document. This is useful for when the
> UI wants to provide the user with information about what has been blocked.
>
> So for style links that fail to load because their URLs were found in the
> blacklist we want to do this annotation here.
>
> (1) I'm covering my basis.
>
> (2) Yes. This includes prefetching. I observed that usually the prefetching
> sheet load was bundled with the actual load so that the first mOwningElement
> was null and the second (in the linked list) was the actual link node.
> AddBlockedTrackingNode() handles null pointers just fine (checks and returns
> immediately)
https://bug1041748.bugzilla.mozilla.org/attachment.cgi?id=8459851
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467981 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Attachment #8467981 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8469651 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Attachment #8468790 -
Flags: review?(mmc)
Reporter | ||
Comment 21•10 years ago
|
||
Comment on attachment 8468790 [details] [diff] [review]
test elements are now actually loaded when tracking protection is disabled/broken. added style loader test
Review of attachment 8468790 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedFrame.html
@@ +24,5 @@
> ];
>
> function checkLoads() {
> + window.parent.is(scriptItem1, "untouched", "Should not load tracking javascript");
> + window.parent.is(scriptItem2, "untouched", "Should not load tracking javascript (2)");
over 80
Attachment #8468790 -
Flags: review?(mmc) → review+
Reporter | ||
Comment 22•10 years ago
|
||
Don't forget to set r=reviewer in all of the patch descriptions please.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8468790 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8467453 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8469651 -
Attachment is obsolete: true
Attachment #8469651 -
Flags: review?(dbaron)
Assignee | ||
Comment 26•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8470110 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8470109 -
Attachment is obsolete: true
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8470107 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8470111 -
Flags: review?(dbaron)
Reporter | ||
Comment 29•10 years ago
|
||
Comment on attachment 8470112 [details] [diff] [review]
object loader now annotates classified tracking nodes
carrying over r+ from comment 15.5
Attachment #8470112 -
Flags: review+
Reporter | ||
Comment 30•10 years ago
|
||
Comment on attachment 8470113 [details] [diff] [review]
test elements are now actually loaded when tracking protection is disabled/broken. added style loader test
carrying over r+ from comment 21
Attachment #8470113 -
Flags: review+
Comment 31•10 years ago
|
||
Comment on attachment 8470111 [details] [diff] [review]
style loader now annotates classified tracking nodes
So assuming that you're ok with no blocked tracking node being added for @import sheets that are blocked, I guess this is ok, though I'm still not entirely sure how severe false negatives are here.
I thought about asking you to move the handling to the existing loop over data in Loader::DoSheetComplete, but that would have the side-effect that the @import-blocked status might or might not propagate to the parent depending on whether it was the last child sheet remaining for the parent to be complete, which wouldn't be good.
(I kinda wish bz was around to review this since he knows this code substantially better than I do, but I also don't want to delay you or to leave a pile of reviews around for bz when he gets back from vacation.)
It might not be a bad idea to add a code comment that |cont| might be null, but AddBlockedTrackingNode is ok with that. Also, please rename that variable from cont to content.
r=dbaron with that
Attachment #8470111 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8470111 -
Attachment is obsolete: true
Assignee | ||
Comment 33•10 years ago
|
||
Reporter | ||
Comment 34•10 years ago
|
||
I took the liberty of fixing a missing <body> tag in test_classified_annotations.html and also rebased on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf21c0388a3, since adding image loads with the same URL was hitting the cache.
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bde52b0b5431
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b1a569d165
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3088e9079d8b
Comment 35•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bde52b0b5431
https://hg.mozilla.org/mozilla-central/rev/c9b1a569d165
https://hg.mozilla.org/mozilla-central/rev/3088e9079d8b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•