propagate NS_ERROR_TRACKING_URI channel cancellations to UI

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mmc, Assigned: geko1702+bugzilla)

Tracking

Trunk
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 27 obsolete attachments)

11.95 KB, patch
mmc
: review+
Details | Diff | Splinter Review
60.53 KB, patch
mmc
: review+
Details | Diff | Splinter Review
We need per-tab notifications when channels have been cancelled with NS_ERROR_TRACKING_URI.
Assignee: nobody → gkontaxis
Blocks: 1029886
Summary: propogate NS_ERROR_TRACKING_URI channel cancellations to UI → propagate NS_ERROR_TRACKING_URI channel cancellations to UI
(Assignee)

Comment 1

5 years ago
Posted patch annotate blocked nodes (obsolete) — Splinter Review
(Assignee)

Comment 2

5 years ago
(Assignee)

Comment 4

5 years ago
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 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.
(Assignee)

Updated

5 years ago
Attachment #8447567 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8447563 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8445578 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8446045 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8447565 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8445578 - Attachment is patch: false
(Assignee)

Updated

5 years ago
Attachment #8445578 - Attachment is patch: true
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
(Assignee)

Updated

5 years ago
Attachment #8448357 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8448467 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8448489 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Posted patch mochitest (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8449081 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8449083 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Posted patch mochitest (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8449082 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8449592 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8450601 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8450603 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
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)
Attachment #8450608 - Flags: review?(khuey)
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.
Attachment #8450608 - Flags: review?(khuey) → review-
(Assignee)

Comment 23

5 years ago
Posted patch mochitest (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8449593 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Posted patch mochitest (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8454036 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8450608 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8454039 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 1037213
(Assignee)

Updated

5 years ago
Attachment #8454055 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8454100 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Posted patch mochitest (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8454038 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
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)
Attachment #8454152 - Flags: review?(khuey)
(Assignee)

Comment 32

5 years ago
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.
Attachment #8454163 - Flags: review?(khuey)
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?
Attachment #8454152 - Flags: review?(khuey)
Attachment #8454152 - Flags: review?(bugs)
Attachment #8454152 - Flags: review+
Comment on attachment 8454163 [details] [diff] [review]
mochitest

I don't have time to review this.
Attachment #8454163 - Flags: review?(khuey) → review?(mmc)
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.
Attachment #8454163 - Flags: review?(mmc) → review-
(Assignee)

Updated

5 years ago
Attachment #8454163 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8457515 - Attachment is obsolete: true
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
Attachment #8454152 - Flags: review?(bugs) → review+
(Assignee)

Updated

5 years ago
Attachment #8454152 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8457517 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8457510 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8457621 - Flags: review?(mmc)
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.
Attachment #8457621 - Flags: review?(mmc) → review+
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.
Attachment #8457620 - Flags: review+
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
(Assignee)

Updated

5 years ago
Attachment #8457621 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a7ee14e17339
https://hg.mozilla.org/mozilla-central/rev/0416b6a1ba2d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.