1029887 - propagate NS_ERROR_TRACKING_URI channel cancellations to UI

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

We need per-tab notifications when channels have been cancelled with NS_ERROR_TRACKING_URI.

Comment 1

[Georgios Kontaxis]

Attachment: annotate blocked nodes

Comment 2

[Georgios Kontaxis]

Attachment: annotated blocked nodes now include images

Comment 3

[Georgios Kontaxis]

Attachment: minor fix (typo) in the image annotation code

Comment 4

[Georgios Kontaxis]

Attachment: annotated blocked nodes now include iframes

Comment 5

[Georgios Kontaxis]

Attachment: HTMLDocument exposes .blackSheepNodeCount, Node actually implements isBlackSheepNode()

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8445578 [details] [diff] [review]
annotate blocked nodes

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

::: dom/base/nsPIDOMWindow.h
@@ +818,5 @@
>    bool mHasNotifiedGlobalCreated;
>  
>    uint32_t mMarkedCCGeneration;
> +
> +public:

private

@@ +826,5 @@
> +   * the URL classifier (SafeBrowsing)
> +   *
> +   * array of weak pointers to allow nodes to disappear
> +   */
> +  nsTArray<nsWeakPtr> blackSheepNodes;

In some modules, member variables start with m, not sure about DOM.

This might be mBlockedTrackingNodes.

Also please document the type of pointer.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8446045 [details] [diff] [review]
annotated blocked nodes now include images

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

::: image/src/imgStatusTracker.h
@@ +188,5 @@
>  
>    // Get the current image status (as in imgIRequest).
>    uint32_t GetImageStatus() const;
>  
> +  uint32_t GetImageErrorCode() const;

Needs comment that this function is for preserving the original error code, if existing code is being overwritten.

Comment 8

[Georgios Kontaxis]

Attachment: extended nsPIDOMWindow with a BlockedTrackingNodes array containing weak references to document nodes that have been blocked for tracking by the url classifier. implemented logic in script, image and iframe loaders to append such blocked elements to the B

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8448357 [details] [diff] [review]
extended nsPIDOMWindow with a BlockedTrackingNodes array containing weak references to document nodes that have been blocked for tracking by the url classifier. implemented logic in script, image and iframe loaders to append such blocked elements to the B

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

Good start, this should be ready for review soon.

::: content/base/src/nsDocument.cpp
@@ +6020,5 @@
>  
> +/*
> + * returns the size of the blockedTrackingNodes array (nsPIDOMWindow)
> + *
> + * This array contains nodes that have been blocked to prevent 

Please set your editor to highlight trailing whitespace and try to get close to 80 columns without going over.

@@ +6041,5 @@
> +  return win->GetBlockedTrackingNodesArray()->Length();
> +}
> +
> +/* 
> + * returns strong references to nodes 

I wonder if the semantics of this ought to be changed, i.e. an array of weak references should be returned and it's up to the caller to get strong references when those are actually in use. This prevents GC for a shorter amount of time (and prevents the caller for preventing GC).

@@ +6049,5 @@
> +already_AddRefed<nsSimpleContentList>
> +nsDocument::BlockedTrackingNodes()
> +{
> +  /* aux */
> +  unsigned long i;

No need to use outside the loop, just do

for (unsigned long i; i < ...)

@@ +6051,5 @@
> +{
> +  /* aux */
> +  unsigned long i;
> +
> +  /* current window */

I notice that in this file, inline comments begin with //. Please be consistent with surrounding code.

@@ +6061,5 @@
> +  /* 
> +   * new list created for holding strong 
> +   * references to blockedTrackingNodes nodes 
> +   */
> +  nsRefPtr<nsSimpleContentList> list;

Declare near where you use.

nsRefPtr<nsSimpleContentList> list = new nsSimpleContentList(nullptr);

etc.

@@ +6063,5 @@
> +   * references to blockedTrackingNodes nodes 
> +   */
> +  nsRefPtr<nsSimpleContentList> list;
> +
> +  /* aux */

aux is not a useful comment.

@@ +6064,5 @@
> +   */
> +  nsRefPtr<nsSimpleContentList> list;
> +
> +  /* aux */
> +  nsWeakPtr weak_node;

Be consistent with naming. If surrounding vars uses camel case, then so should all variable names.

@@ +6068,5 @@
> +  nsWeakPtr weak_node;
> +  nsCOMPtr<nsIContent> node;
> +
> +  /* 
> +   * create new content list to 

line too short

@@ +6085,5 @@
> +  for (i = 0; i < _blockedTrackingNodes.Length(); i++)
> +  {
> +    weak_node = _blockedTrackingNodes[i];
> +    /* strong reference */
> +    node = do_QueryReferent(weak_node);

Need to check the return value of QueryReferent, either by

nsresult rv;
node = do_QueryReferent(weakNode, &rv);
NS_ENSURE_SUCCESS(rv, rv); // returns early if the previous failed
rv = list->AppendElement(node);
...

::: content/base/src/nsDocument.h
@@ +1133,5 @@
>    virtual void SetFullscreenRoot(nsIDocument* aRoot) MOZ_OVERRIDE;
>  
> +  long BlockedTrackingNodeCount();
> +
> +  already_AddRefed<nsSimpleContentList> BlockedTrackingNodes();

Function names should be actions, like GetBlockedTrackingNodes. I'm also confused why this isn't part of the idl, but then again I don't understand why there's not nsIDocument.idl in the first place.

::: content/base/src/nsImageLoadingContent.cpp
@@ +163,5 @@
>    if (aType == imgINotificationObserver::LOAD_COMPLETE) {
>      uint32_t reqStatus;
>      aRequest->GetImageStatus(&reqStatus);
> +    /* triage STATUS_ERROR */
> +    uint32_t errorCode;

Change the signature to GetImageErrorCode(nsresult* rv), then do 

nsresult errorCode;

and you can skip the cast below.

@@ +164,5 @@
>      uint32_t reqStatus;
>      aRequest->GetImageStatus(&reqStatus);
> +    /* triage STATUS_ERROR */
> +    uint32_t errorCode;
> +    if (reqStatus & imgIRequest::STATUS_ERROR)

begin braces go on the same line

@@ +173,5 @@
> +       * source was a tracking URL (SafeBrowinsg) 
> +       *
> +       * we make a note of this image node by 
> +       * including it in a dedicated array of 
> +       * blocked tracking nodes under its own 

Lots of trailing whitespace, columns too short, "Safebrowsing"

@@ +181,5 @@
> +      {
> +        // get the document owning this image
> +        nsIDocument *doc = GetOurOwnerDoc();
> +
> +        if (doc)

Too much nesting.

@@ +198,5 @@
> +              // get a weak reference for this node and append 
> +              // that reference to the window array of blocked 
> +              // tracking nodes. can be used later on to look up 
> +              // a node against it (e.g., by the UI)
> +              nsWeakPtr weakNode = do_GetWeakReference(thisNode);

Check return.

::: docshell/base/nsDocShell.cpp
@@ +7060,5 @@
> +        if (isTopFrame == false && aStatus == NS_ERROR_TRACKING_URI)
> +        {
> +          nsPIDOMWindow *window = nullptr;
> +
> +          // Get a handle to the root docshell (root window)

Delete unused code.

::: dom/base/nsPIDOMWindow.h
@@ +393,5 @@
> +   * of blocked tracking nodes found in this window
> +   */
> +  nsTArray<nsWeakPtr> *GetBlockedTrackingNodesArray()
> +  {
> +    return &mBlockedTrackingNodes;

This seems pretty dangerous! Since we're only using the length for now, better just to expose the length and not a handle to a protected member variable.

::: image/src/imgStatusTracker.cpp
@@ +883,5 @@
>  
>    // If we were successful in loading, note that the image is complete.
>    if (NS_SUCCEEDED(aStatus) && mImageStatus != imgIRequest::STATUS_ERROR)
>      mImageStatus |= imgIRequest::STATUS_LOAD_COMPLETE;
>    else

beginning braces (except for at the beginning of a function) go on the same line

::: image/src/imgStatusTracker.h
@@ +336,5 @@
> +  // his image has been blocked on purpose 
> +  // (Safebrowsing)
> +  // 
> +  // see xpcom/base/ErrorList.h
> +  uint32_t mImageErrorCode;

nsresult

Comment 10

[Georgios Kontaxis]

Attachment: extended nsPIDOMWindow with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent window. exposed window.GetBlockedTrackingNodes and window.GetBlockedTrackingNodesCount

Comment 11

[Georgios Kontaxis]

Attachment: extended nsPIDOMWindow with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent window. exposed attribs window.blockedTrackingNodes and window.blockedTrackingNodesCount

Comment 12

[Georgios Kontaxis]

Attachment: extended nsPIDOMWindow with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent window. exposed attribs window.blockedTrackingNodes and window.blockedTrackingNodesCount

Comment 13

[Georgios Kontaxis]

Attachment: mochitest

Comment 14

[Georgios Kontaxis]

Attachment: extended nsPIDOMWindow with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent window. exposed attribs window.blockedTrackingNodes and window.blockedTrackingNodesCount

Comment 15

[Georgios Kontaxis]

Attachment: extended nsPIDOMWindow with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent window. exposed attribs window.blockedTrackingNodes and window.blockedTrackingNodesCount

Comment 16

[Georgios Kontaxis]

Attachment: mochitest

Comment 17

[Georgios Kontaxis]

Attachment: extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

Comment 18

[Georgios Kontaxis]

Attachment: extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

Comment 19

[Georgios Kontaxis]

Attachment: extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

Comment 20

[Georgios Kontaxis]

Comment on attachment 8450608 [details] [diff] [review]
extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

This patch annotates scripts, images and iframes that fail to load because of Safebrowsing anti-tracking protection (See https://bugzilla.mozilla.org/show_bug.cgi?id=1024610) and exposes a JS API to get their number and strong references to them which is something useful for building a UI for Bug 1024610.

Annotation happens by maintaining an array of weak references to the respective blocked nodes (nsIContent instances) in their parent document. (nsIDocument)

JS API is exposed under window.document (Document.webidl)

Comment 21

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=1142b754c743

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8450608 [details] [diff] [review]
extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

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

Why do you want to get the count separately?

Please follow conventions of written English such as capitalizing the first letter of a sentence and ending it with a period in your comments.

::: content/base/public/nsIDocument.h
@@ +2659,5 @@
> +   * classifier (Safebrowsing).
> +   *
> +   * weak nsINode pointers are used to allow nodes to disappear
> +   */
> +  nsTArray<nsWeakPtr> mBlockedTrackingNodes;

Nope, no public member variables.  Add a function that takes a node and grabs the weak pointer and puts it in the array.

::: content/base/src/nsDocument.cpp
@@ +6009,5 @@
> +nsDocument::BlockedTrackingNodes()
> +{
> +  // new content list to hold strong references to mBlockedTrackingNodes nodes
> +  nsRefPtr<nsSimpleContentList> list = new nsSimpleContentList(nullptr);
> +  NS_ASSERTION(list, "NS_OUT_OF_MEMORY");

new Foo can never fail in Gecko, so this is unnecessary.

@@ +6013,5 @@
> +  NS_ASSERTION(list, "NS_OUT_OF_MEMORY");
> +
> +  // snapshot of pointers in mBlockedTrackingNodes array
> +  nsTArray<nsWeakPtr> _blockedTrackingNodes;
> +  _blockedTrackingNodes = mBlockedTrackingNodes;

we don't like underscores.  Just blockedTrackingNodes is fine.

::: content/base/src/nsImageLoadingContent.cpp
@@ +163,5 @@
>    if (aType == imgINotificationObserver::LOAD_COMPLETE) {
>      uint32_t reqStatus;
>      aRequest->GetImageStatus(&reqStatus);
> +    /* triage STATUS_ERROR */
> +    nsresult errorCode;

Declare this inside the if.

@@ +187,5 @@
> +          weakNode = do_GetWeakReference(thisNode);
> +        }
> +
> +        // get the document owning this image
> +        nsIDocument *doc = GetOurOwnerDoc();

This can never return null.

::: content/base/src/nsScriptLoader.cpp
@@ +1419,5 @@
> +      // document. can be used later on to look up a node in it
> +      // (e.g., by the UI)
> +      nsWeakPtr weakNode = do_GetWeakReference(request->mElement);
> +
> +      if (weakNode && this->mDocument) {

The scriptloader always has a document, no need to null check.

@@ +1420,5 @@
> +      // (e.g., by the UI)
> +      nsWeakPtr weakNode = do_GetWeakReference(request->mElement);
> +
> +      if (weakNode && this->mDocument) {
> +        this->mDocument->mBlockedTrackingNodes.AppendElement(weakNode);

Don't do this->mFoo->..., just do mFoo->...

::: docshell/base/nsDocShell.cpp
@@ +7059,5 @@
> +        * blocked document)
> +        */
> +        if (isTopFrame == false && aStatus == NS_ERROR_TRACKING_URI) {
> +          // frameElement is our nsIContent to be annotated
> +          nsCOMPtr<nsIDOMElement> frameElement = nullptr;

Smart pointers initialize themselves to null.

@@ +7060,5 @@
> +        */
> +        if (isTopFrame == false && aStatus == NS_ERROR_TRACKING_URI) {
> +          // frameElement is our nsIContent to be annotated
> +          nsCOMPtr<nsIDOMElement> frameElement = nullptr;
> +          nsPIDOMWindow *thisWindow = this->GetWindow();

Just GetWindow().

::: dom/webidl/HTMLDocument.webidl
@@ +35,5 @@
> +  /*
> +   * number of nodes that have been blocked by
> +   * the Safebrowsing API to prevent tracking
> +   */
> +  readonly attribute long blockedTrackingNodeCount;

These new properties need to be [ChromeOnly].  Please move them to a separate partial interface just for them (see Document.webidl for some examples).

@@ +41,5 @@
> +  /*
> +   * list of nodes that have been blocked by
> +   * the Safebrowsing API to prevent tracking
> +   */
> +  readonly attribute NodeList blockedTrackingNodes;

here too.

This should also return a sequence<Node> rather than a NodeList.

::: image/public/imgIRequest.idl
@@ +82,5 @@
> +  /*
> +   * actual error code that generated a STATUS_ERROR imageStatus
> +   * (see xpcom/base/ErrorList.h)
> +   */
> +  readonly attribute nsresult imageErrorCode;

A peer of imagelib will have to sign off on this, but it looks reasonable to me.

Please update the IID for this interface.

::: image/src/imgStatusTracker.cpp
@@ +884,5 @@
>    // If we were successful in loading, note that the image is complete.
>    if (NS_SUCCEEDED(aStatus) && mImageStatus != imgIRequest::STATUS_ERROR)
>      mImageStatus |= imgIRequest::STATUS_LOAD_COMPLETE;
>    else
> +  {

while you're here, put braces around the if, and cuddle the else

::: image/src/imgStatusTracker.h
@@ +334,5 @@
>    uint32_t mImageStatus;
> +  // mImageStatus == STATUS_ERROR is not enough. For instance we might want
> +  // to know that his image has been blocked on purpose (Safebrowsing)
> +  //
> +  // see xpcom/base/ErrorList.h

You don't need this comment, it's obvious what this does.

Comment 23

[Georgios Kontaxis]

Attachment: mochitest

Comment 24

[Georgios Kontaxis]

Attachment: mochitest

Comment 25

[Georgios Kontaxis]

Attachment: extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

* * *
try: -b do -p linux,macosx64,win32,android,emulator -u all -t none

Comment 26

[Georgios Kontaxis]

Attachment: extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

* * *
try: -b do -p linux,macosx64,win32,android,emulator -u all -t none

Comment 27

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=dfc193adfb21

Comment 28

[Georgios Kontaxis]

Attachment: extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

* * *
try: -b do -p linux,macosx64,win32,android,emulator -u all -t none

Comment 29

[Georgios Kontaxis]

Attachment: extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

* * *
try: -b do -p linux,macosx64,win32,android,emulator -u all -t none

Comment 30

[Georgios Kontaxis]

Attachment: mochitest

Comment 31

[Georgios Kontaxis]

Comment on attachment 8454152 [details] [diff] [review]
extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

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

extended nsIDocument with protected BlockedTrackingNodes to keep nsIContent nodes blocked by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed ChromeOnly JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

This patch annotates scripts, images and iframes that fail to load because of Safebrowsing anti-tracking protection (See https://bugzilla.mozilla.org/show_bug.cgi?id=1024610) and exposes a JS API to get their number and strong references to them which is something useful for building a UI for Bug 1024610.

Image annotation requires supporting changes in imagelib. Under review. https://bugzilla.mozilla.org/show_bug.cgi?id=1037213

Annotation happens by maintaining an array of weak references to the respective blocked nodes (nsIContent instances) in their parent document. (nsIDocument)

JS API is exposed under window.document (HTMLDocument.webidl)

Comment 32

[Georgios Kontaxis]

Comment on attachment 8454163 [details] [diff] [review]
mochitest

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

test that blocked DOM element annotation happens correctly. Tries to load elements using URIs that will be blocked and makes sure that all (and only those) such elements are annotated.

Comment 33

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=5acb7352a03d

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8454152 [details] [diff] [review]
extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

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

smaug should look at the docshell bits.

This patch needs a better commit message before it is landed.

::: content/base/public/nsIDocument.h
@@ +2651,5 @@
>  
>    uint32_t mBlockDOMContentLoaded;
>    bool mDidFireDOMContentLoaded:1;
> +
> +protected:

The stuff above this is already protected.  You don't need to specify that again.

@@ +2658,5 @@
> +  // They most likely have had their nsIChannel canceled by the URL
> +  // classifier. (Safebrowsing)
> +  //
> +  // Weak nsINode pointers are used to allow nodes to disappear.
> +  nsTArray<nsWeakPtr> mBlockedTrackingNodes;

I would prefer that you put this next to mFrameRequestCallbacks just to make sure that we don't have any structure padding issues.

@@ +2666,5 @@
> +   * Given a node, get a weak reference to it and append that reference to
> +   * mBlockedTrackingNodes. Can be used later on to look up a node in it.
> +   * (e.g., by the UI)
> +   */
> +  void AddBlockedTrackingNode (nsINode *node)

no space before (

Move this function up to below ImportManager()

::: content/base/src/nsDocument.cpp
@@ +5990,5 @@
> + * document since its beginning, not how many of them are still around
> + * in the DOM tree. Weak references to blocked nodes are added in the
> + * mBlockedTrackingNodesArray but they are not removed when those nodes
> + * are removed from the tree or even garbage collected.
> + */

You don't need to duplicate this comment.

@@ +6003,5 @@
> + *
> + * This array contains nodes that have been blocked to prevent
> + * user tracking. They most likely have had their nsIChannel
> + * canceled by the URL classifier (Safebrowsing).
> + */

Same here

@@ +6008,5 @@
> +already_AddRefed<nsSimpleContentList>
> +nsDocument::BlockedTrackingNodes()
> +{
> +  // New content list to hold strong references to mBlockedTrackingNodes nodes
> +  nsRefPtr<nsSimpleContentList> list = new nsSimpleContentList(nullptr);

You don't need to comment things that are easily discerned from reading the code such as this.

@@ +6012,5 @@
> +  nsRefPtr<nsSimpleContentList> list = new nsSimpleContentList(nullptr);
> +
> +  // Snapshot of pointers in mBlockedTrackingNodes array
> +  nsTArray<nsWeakPtr> blockedTrackingNodes;
> +  blockedTrackingNodes = mBlockedTrackingNodes;

and this.

@@ +6018,5 @@
> +  for (unsigned long i = 0; i < blockedTrackingNodes.Length(); i++) {
> +    nsWeakPtr weakNode = blockedTrackingNodes[i];
> +    nsCOMPtr<nsIContent> node = do_QueryReferent(weakNode);
> +    // Consider only nodes to which we have managed to get strong references.
> +    // Coping with nullptrs since it's expected from nodes to disappear when

expected for

@@ +6026,5 @@
> +    }
> +  }
> +
> +  // Transfer list reference
> +  return list.forget();

and this.

::: content/base/src/nsDocument.h
@@ +1158,5 @@
> +  // document since its beginning, not how many of them are still around
> +  // in the DOM tree. Weak references to blocked nodes are added in the
> +  // mBlockedTrackingNodesArray but they are not removed when those nodes
> +  // are removed from the tree or even garbage collected.
> +  long BlockedTrackingNodeCount();

This should be const

@@ +1167,5 @@
> +  // This array contains nodes that have been blocked to prevent
> +  // user tracking. They most likely have had their nsIChannel
> +  // canceled by the URL classifier (Safebrowsing).
> +  //
> +  already_AddRefed<nsSimpleContentList> BlockedTrackingNodes();

Same here.

::: content/base/src/nsImageLoadingContent.cpp
@@ +165,5 @@
>      aRequest->GetImageStatus(&reqStatus);
> +    /* triage STATUS_ERROR */
> +    if (reqStatus & imgIRequest::STATUS_ERROR) {
> +      nsresult errorCode;
> +      aRequest->GetImageErrorCode(&errorCode);

You should initialize errorCode or check the return value of this function.  If GetImageErrorCode fails then you're going to jump on an uninitialized variable.

@@ +173,5 @@
> +       * in a dedicated array of blocked tracking nodes under its parent
> +       * document.
> +       */
> +      if (errorCode == NS_ERROR_TRACKING_URI) {
> +        /* Reach nsIContent for this image. */

Another comment that is easily discerned from reading code.

@@ +177,5 @@
> +        /* Reach nsIContent for this image. */
> +        nsCOMPtr<nsIContent> thisNode
> +          = do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
> +
> +        /* Get the document owning this image. */

here too.

@@ +178,5 @@
> +        nsCOMPtr<nsIContent> thisNode
> +          = do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
> +
> +        /* Get the document owning this image. */
> +        nsIDocument *doc = GetOurOwnerDoc();

As I said before, this can never return null, so there's no need to null check it.

::: docshell/base/nsDocShell.cpp
@@ +7063,5 @@
> +          nsCOMPtr<nsIDOMElement> frameElement;
> +          nsPIDOMWindow *thisWindow = GetWindow();
> +          if (thisWindow) {
> +            thisWindow->GetFrameElement(getter_AddRefs(frameElement));
> +          }

If we don't have a frameElement we should skip everything after this right?

Comment 35

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8454163 [details] [diff] [review]
mochitest

I don't have time to review this.

Comment 36

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8454163 [details] [diff] [review]
mochitest

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

Looking good! Just a few fixups.

::: toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedFrame.html
@@ +4,5 @@
> +
> +<script type="text/javascript">
> +
> +var scriptItem = "untouched";
> +var  imageItem = "untouched";

Don't line up indentation on = or other separators. In the long run it is unsustainable and leads to very ugly indentation. Single space is fine.

@@ +17,5 @@
> +  "badframe2"
> +];
> +
> +function checkLoads() {
> +  // Make sure the javascript did not load.

These "Make sure" comments are redundant because is() is already documenting them and spitting out a relevant error when it fails.

@@ +31,5 @@
> +  // We expect a count of 4: 2 scripts, 1 image, 1 iframe
> +  window.parent.is(window.document.blockedTrackingNodeCount, badids.length,
> +    "Should identify all tracking elements");
> +
> +  var blockedTrackingNodes = window.document.blockedTrackingNodes;

Best practice is to use smallest scope (let instead of var) unless you explicitly need function scope.

@@ +36,5 @@
> +
> +  // Make sure that every node in blockedTrackingNodes exists in the tree
> +  // (that may not always be the case but do not expect any nodes to disappear
> +  // from the tree here)
> +  var AllNodeMatch = true;

camelCase in the rest of this file and elsewhere

allNodeMatch

@@ +40,5 @@
> +  var AllNodeMatch = true;
> +  for (var i = 0; i < blockedTrackingNodes.length; i++) {
> +    var NodeMatch = false;
> +    for (var j = 0; j < badids.length; j++) {
> +      NodeMatch |=

Can these lines fit in 80? If so, too many newlines.

Also this logic could shortcut.

if (blockedTrackingNodes[i] == document.getElementById(badids[j]) {
  foundNode = true;
  break;
}

foundAllNodes &= foundNode

@@ +49,5 @@
> +    AllNodeMatch &= NodeMatch;
> +  }
> +  window.parent.is(AllNodeMatch, true,
> +    "All annotated nodes are expected in the tree");
> +

I think the loop below is redundant, no? You can change allNodeMatch to numNodesFound and at the end check

do_check_eq(numNodesFound, badids.length)

That way you are guaranteed that all of the badids are found.

@@ +51,5 @@
> +  window.parent.is(AllNodeMatch, true,
> +    "All annotated nodes are expected in the tree");
> +
> +  // Make sure that every node with a badid (see badids) is found in the
> +  // blocekdTrackingNodes. This tells us if we are neglecting to annotate

s/blocekd/blocked

@@ +77,5 @@
> +</head>
> +
> +<body onload="checkLoads()">
> +
> +<!-- Try loading from a tracking scipt URI (1) -->

s/scipt/script

@@ +87,5 @@
> +<!-- Try loading from a tracking frame URI (1) -->
> +<iframe id="badframe1" src="http://tracking.example.com/tests/toolkit/components/url-classifier/tests/mochitest/track.html" onload="function() {frameItem='spoiled';}"></iframe>
> +
> +<script>
> +// Try loading from a tracking script URI (2) - This is different in terms of its nsScriptLoader

s/This is different/The loader path may be different depending on whether the resource is loaded from JS or HTML.

Comment 37

[Georgios Kontaxis]

Attachment: try: -b do -p linux,macosx64,win32,android,emulator -u all -t none

Comment 38

[Georgios Kontaxis]

Attachment: extended n keep track of DOM nodes that have had their URL classified as NS_TRACKING_URI (related nsIChannel canceled). nsIDocument holds references to such nodes. Logic in script, img and iframe loaders to create references in their parent document. Expo

Comment 39

[Georgios Kontaxis]

Attachment: keep track of DOM nodes that have had their URL classified as NS_TRACKING_URI (related nsIChannel canceled). nsIDocument holds references to such nodes. Logic in script, img and iframe loaders to create references in their parent document. Exposed JS attr

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8454152 [details] [diff] [review]
extended nsIDocument with BlockedTrackingNodes by the url-classifier. logic in script, img, iframe loaders to append blocked instances to their parent document. exposed to JS attribs document.blockedTrackingNodes and document.blockedTrackingNodesCount

>@@ -7046,16 +7046,47 @@ nsDocShell::EndPageLoad(nsIWebProgress *
Docshell is an odd beast and uses 4 spaces indentation 


>     if (url && NS_FAILED(aStatus)) {
>         if (aStatus == NS_ERROR_FILE_NOT_FOUND ||
>             aStatus == NS_ERROR_CORRUPTED_CONTENT ||
>             aStatus == NS_ERROR_INVALID_CONTENT_ENCODING) {
>             DisplayLoadError(aStatus, url, nullptr, aChannel);
>             return NS_OK;
>         }
> 
>+       /*
>+        * Handle iframe document not loading error because source was
>+        * a tracking URL. (Safebrowinsg) We make a note of this iframe
Safebrowsing


>+        if (isTopFrame == false && aStatus == NS_ERROR_TRACKING_URI) {
>+          // FrameElement is our nsIContent to be annotated
>+          nsCOMPtr<nsIDOMElement> frameElement;
>+          nsPIDOMWindow *thisWindow = GetWindow();
* goes with the type. So nsPIDOMWindow*

>+          if (thisWindow) {
>+            thisWindow->GetFrameElement(getter_AddRefs(frameElement));
>+          }
Return early if thisWindow is null

>+
>+          // Parent window
>+          nsCOMPtr<nsIDocShellTreeItem> parentItem;
>+          GetSameTypeParent(getter_AddRefs(parentItem));
>+          // Parent document
>+          nsCOMPtr<nsIDocument> parentDoc;
>+          if (parentItem) {
>+            parentDoc = parentItem->GetDocument();
>+          }
return early if no parentDoc

Comment 41

[Georgios Kontaxis]

Attachment: keep track of DOM nodes that have had their URL classified as NS_TRACKING_URI (related nsIChannel canceled). nsIDocument holds references to such nodes. Logic in script, img and iframe loaders to create references in their parent document. Exposed JS attr

Comment 42

[Georgios Kontaxis]

Attachment: test bookkeeping of DOM nodes that have had their URL classified as NS_TRACKING_URI

Comment 43

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=fd51165bf328

Comment 44

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=12f7864c1899

Comment 45

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8457621 [details] [diff] [review]
test bookkeeping of DOM nodes that have had their URL classified as NS_TRACKING_URI

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

After discussing, the double loop is OK. Also sorry about suggesting let! I thought that would work.

Comment 46

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8457620 [details] [diff] [review]
keep track of DOM nodes that have had their URL classified as NS_TRACKING_URI (related nsIChannel canceled). nsIDocument holds references to such nodes. Logic in script, img and iframe loaders to create references in their parent document. Exposed JS attr

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

Carrying over r+ from khuey and smaug in comment 34 and comment 40.

Comment 47

[[:mmc] Monica Chew (no longer reading bugmail)]

Georgios, please repush to try with minimal set of changes (no UI changes) so we can verify they don't break customizableUI tests:

https://tbpl.mozilla.org/php/getParsedLog.php?id=43977066&tree=Try#error0

Comment 48

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=84583d66536f

Comment 49

[Georgios Kontaxis]

Attachment: test bookkeeping of DOM nodes that have had their URI classified as NS_TRACKING_URI

Comment 50

[[:mmc] Monica Chew (no longer reading bugmail)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ee14e17339
 https://hg.mozilla.org/integration/mozilla-inbound/rev/0416b6a1ba2d

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7ee14e17339
https://hg.mozilla.org/mozilla-central/rev/0416b6a1ba2d