1608516 - Update AddBlockedNodeByClassifier on subframes for Fission

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect, P3)

Description

[Nika Layzell [:nika] (ooo, ni? for response)]

Tracking Protection appears to record that a frame element's load was blocked by the classifier from within that element. This is not compatible with Fission, as the frame element may not be present in the same process as the embedded nsDocShell. https://searchfox.org/mozilla-central/rev/d4d6f81e0ab479cde192453bae83d5e3edfb39d6/docshell/base/nsDocShell.cpp#6158-6192

Comment 1

[Steven Englehardt [:englehardt]]

ni Tim to get this on his radar.

Comment 2

[Tim Huang[:timhuang]]

I believe we can move the blockedNodesByClassifier and the blockedNodeByClassifierCount from content processes to the parent process. And move them from the document to the windowGlobalParent. These two values are chrome only and we are only using them in the test, so I think it is totally fine to move them.

Comment 3

[(no longer active)]

We can't move blockedNodesByClassifier to the parent, since it returns a NodeList. The nodes in that list live in the content process, it's meaningless to return a list of them in the parent process...

My counter proposal is to remove both of these properties and the code as well as the tests using them. Is there any reason why we want to keep them around? Removing them seems much simpler than making them work in Fission...

Comment 4

[Tim Huang[:timhuang]]

(In reply to :ehsan akhgari from comment #3)

We can't move blockedNodesByClassifier to the parent, since it returns a NodeList. The nodes in that list live in the content process, it's meaningless to return a list of them in the parent process...

Thanks for catching this. I didn't see this before. So, yes, moving them to the parent is not a possible solution.

My counter proposal is to remove both of these properties and the code as well as the tests using them. Is there any reason why we want to keep them around? Removing them seems much simpler than making them work in Fission...

Removing them is an effective solution here. But, see here, the test verifies the annotated nodes through blockedNodesByClassifier and I think a test like this is useful. So, maybe we can remove them here and add similar tests by using a Fission compatible approach in a different bug?

What do you thank, Ehsan?

Comment 5

[(no longer active)]

(In reply to Tim Huang[:timhuang] from comment #4)

Removing them is an effective solution here. But, see here, the test verifies the annotated nodes through blockedNodesByClassifier and I think a test like this is useful. So, maybe we can remove them here and add similar tests by using a Fission compatible approach in a different bug?

What do you thank, Ehsan?

It seems to me that this test is just verifying that document.blockedNodesByClassifier is working as expected. The test is already checking that the correct nodes are being blocked (using the node.dataset.checked checks) and then verifying that document.blockedNodesByClassifier returns the same information.

So as far as I am concerned, we can just remove this part of that test. :-)

Comment 6

[Tim Huang[:timhuang]]

Thanks for clarifying. I will do this.

Comment 7

[Tim Huang[:timhuang]]

I believe we have an alternative for this instead of removing blockedNodesByClassifier. The blockedNodesByClassifier only record the nodes within the same document. And the code mentioned in comment 0 is an iframe to report the blocking to its parent document. The problem here is the parent document might be out-of-process. So, we can address this via sending an IPC to the process of the parent document to tell it to record the blocked node, which is the frame element and it is supposed to be in the parent doucment's process.

Comment 8

[Tim Huang[:timhuang]]

Attachment: Bug 1608516 - Part 1: Add IPCs for allowing do AddBlockedNodeByClassifier() across process boundaries. r?baku!

This patch adds IPCs that allow us to be able to report the blocked node
across the process boundary.

Comment 9

[Tim Huang[:timhuang]]

Attachment: Bug 1608516 - Part 2: Make AddBlockedNodeByClassifier() in nsDocShell::EndPageLoad() Fission-compatible. r?baku!

In this patch, we report the blocked node across the process
boundaries if the parent is in a different process. If the parent
document is in the same process as the reporting document, we
will directly add the blocked node in the parent document without
sending an IPC.

Depends on D71938

Comment 10

[Tim Huang[:timhuang]]

Attachment: Bug 1608516 - Part 3: Add a pref to prevent sending unnecessary IPC if we are not in testing. r?baku!

Depends on D71939

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c1be470cb2c
Part 1: Add IPCs for allowing do AddBlockedNodeByClassifier() across process boundaries. r=baku
https://hg.mozilla.org/integration/autoland/rev/2a8566d55ba6
Part 2: Make AddBlockedNodeByClassifier() in nsDocShell::EndPageLoad() Fission-compatible. r=baku
https://hg.mozilla.org/integration/autoland/rev/f588a8808e45
Part 3: Add a pref to prevent sending unnecessary IPC if we are not in testing. r=baku

Comment 12

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (Bug 1608516) for causing mochitest permafailures in toolkit/components/url-classifier/tests/mochitest/test_classified_annotations.html CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=6cd64b8476d9bb52768183555b5a73821cef5f15

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=9a4b7583842599f8a8ec482097e58408cd60bba7&selectedJob=298909732

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298909732&repo=autoland&lineNumber=112033

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5169541af78
Part 1: Add IPCs for allowing do AddBlockedNodeByClassifier() across process boundaries. r=baku
https://hg.mozilla.org/integration/autoland/rev/41aab5ec35c6
Part 2: Make AddBlockedNodeByClassifier() in nsDocShell::EndPageLoad() Fission-compatible. r=baku
https://hg.mozilla.org/integration/autoland/rev/25445df807ec
Part 3: Add a pref to prevent sending unnecessary IPC if we are not in testing. r=baku

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b5169541af78
https://hg.mozilla.org/mozilla-central/rev/41aab5ec35c6
https://hg.mozilla.org/mozilla-central/rev/25445df807ec