Similar and complementary to bug 1047705

Comment on attachment 8470305 [details] [diff] [review] nsChannelClassifier will update the security state of the document and fire a security state change event to include the nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance Review of attachment 8470305 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsChannelClassifier.cpp @@ +124,5 @@ > + // do update document security state and fire security change event > + // if protection pref is on > + if (!*result && > + Preferences::GetBool("privacy.trackingprotection.enabled", false)) { > + nsCOMPtr<nsPIDOMWindow> pwin = do_QueryInterface(win); &rv @@ +323,5 @@ > "with error code: %x", this, mSuspendedChannel.get(), > spec.get(), aErrorCode)); > #endif > + // blocked something, do update document security state and > + // fire security change event Check status == NS_ERROR_TRACKING_URI before doing anything further. Otherwise this code fires for phishing and malware classifications. @@ +340,5 @@ > + doc->SetHasTrackingContentBlocked(true); > + // Notify nsIWebProgressListeners of this security event. > + // Can be used to change the UI state. > + nsCOMPtr<nsISecurityEventSink> eventSink = > + do_QueryInterface(docShell); do_QI(docShell, &rv); // ensure success @@ +344,5 @@ > + do_QueryInterface(docShell); > + // Read current security state if possible and append to it. > + uint32_t state = 0; > + nsCOMPtr<nsISecureBrowserUI> securityUI; > + docShell->GetSecurityUI(getter_AddRefs(securityUI)); For consistency, since you are using NS_ENSURE_SUCCESS, etc. above, do rv = docShell->GetSecurityUI(...) NS_ENSURE_SUCCESS(rv, rv); Then you can skip the if's below

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #11) > Comment on attachment 8470305 [details] [diff] [review] > nsChannelClassifier will update the security state of the document and fire > a security state change event to include the > nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or > STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance > > Review of attachment 8470305 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/src/nsChannelClassifier.cpp > @@ +124,5 @@ > > + // do update document security state and fire security change event > > + // if protection pref is on > > + if (!*result && > > + Preferences::GetBool("privacy.trackingprotection.enabled", false)) { > > + nsCOMPtr<nsPIDOMWindow> pwin = do_QueryInterface(win); > > &rv > > @@ +323,5 @@ > > "with error code: %x", this, mSuspendedChannel.get(), > > spec.get(), aErrorCode)); > > #endif > > + // blocked something, do update document security state and > > + // fire security change event > > Check status == NS_ERROR_TRACKING_URI before doing anything further. > Otherwise this code fires for phishing and malware classifications. > > @@ +340,5 @@ > > + doc->SetHasTrackingContentBlocked(true); > > + // Notify nsIWebProgressListeners of this security event. > > + // Can be used to change the UI state. > > + nsCOMPtr<nsISecurityEventSink> eventSink = > > + do_QueryInterface(docShell); > > do_QI(docShell, &rv); > // ensure success > > @@ +344,5 @@ > > + do_QueryInterface(docShell); > > + // Read current security state if possible and append to it. > > + uint32_t state = 0; > > + nsCOMPtr<nsISecureBrowserUI> securityUI; > > + docShell->GetSecurityUI(getter_AddRefs(securityUI)); > > For consistency, since you are using NS_ENSURE_SUCCESS, etc. above, do > > rv = docShell->GetSecurityUI(...) > NS_ENSURE_SUCCESS(rv, rv); > > Then you can skip the if's below GetSecurityUI() seems to be always returning NS_OK http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2691 I think checking the pointer after the call is a necessary.

Comment on attachment 8470998 [details] [diff] [review] nsChannelClassifier will update the security state of the document and fire a security state change event to include the nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance Review of attachment 8470998 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsChannelClassifier.cpp @@ +121,5 @@ > > *result = permissions != nsIPermissionManager::ALLOW_ACTION; > + > + // decision is to ENABLE tracking protection or > + // tracking protection pref is OFF, do return immediately Comments should be sentences with appropriate caps and punctuation. @@ +123,5 @@ > + > + // decision is to ENABLE tracking protection or > + // tracking protection pref is OFF, do return immediately > + if (*result || > + !Preferences::GetBool("privacy.trackingprotection.enabled", false)) braces around return. You can also return early (at the top of this function) if the preference is not enabled. @@ +138,5 @@ > + } > + // update document flag > + nsCOMPtr<nsIDocument> doc = do_GetInterface(docShell, &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + doc->SetHasTrackingContentLoaded(true); Do you only want to call onSecurityChange if you were able to successfully call SetHasTrackingContentLoaded? @@ +152,5 @@ > + if (securityUI) { > + securityUI->GetState(&state); > + } > + state |= nsIWebProgressListener::STATE_LOADED_TRACKING_CONTENT; > + eventSink->OnSecurityChange(nullptr, state); If we failed getting the securityUI, we may be inadvertently resetting the state on the eventSink here, unless you have verified that all listeners only OR in the new state (seems unlikely). We probably want to fail early if there's no securityUI. @@ +329,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + nsCOMPtr<nsPIDOMWindow> pwin = do_QueryInterface(win, &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + nsCOMPtr<nsIDocShell> docShell = pwin->GetDocShell(); > + if (!docShell) braces around return ::: netwerk/base/src/nsChannelClassifier.h @@ +29,5 @@ > void MarkEntryClassified(nsresult status); > bool HasBeenClassified(nsIChannel *aChannel); > // Whether or not tracking protection should be enabled on this channel. > nsresult ShouldEnableTrackingProtection(nsIChannel *aChannel, bool *result); > + // Act upon blocking tracking content: update security state of the Better comment is: // If we are blocking tracking content, update the docshell with nsISecurityEventSink::onSecurityChange.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #16) > Comment on attachment 8470998 [details] [diff] [review] > nsChannelClassifier will update the security state of the document and fire > a security state change event to include the > nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or > STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance > > Review of attachment 8470998 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/src/nsChannelClassifier.cpp > @@ +121,5 @@ > > > > *result = permissions != nsIPermissionManager::ALLOW_ACTION; > > + > > + // decision is to ENABLE tracking protection or > > + // tracking protection pref is OFF, do return immediately > > Comments should be sentences with appropriate caps and punctuation. > > @@ +123,5 @@ > > + > > + // decision is to ENABLE tracking protection or > > + // tracking protection pref is OFF, do return immediately > > + if (*result || > > + !Preferences::GetBool("privacy.trackingprotection.enabled", false)) > > braces around return. You can also return early (at the top of this > function) if the preference is not enabled. > > @@ +138,5 @@ > > + } > > + // update document flag > > + nsCOMPtr<nsIDocument> doc = do_GetInterface(docShell, &rv); > > + NS_ENSURE_SUCCESS(rv, rv); > > + doc->SetHasTrackingContentLoaded(true); > > Do you only want to call onSecurityChange if you were able to successfully > call SetHasTrackingContentLoaded? > Yes. OnSecurityChange() will fire an event which may cause the UI to update. Without a persistent docshell flag that state may be quickly lost if something else tries to update the same UI while drawing state from the docshell. (parts of mixed content protection do that) > @@ +152,5 @@ > > + if (securityUI) { > > + securityUI->GetState(&state); > > + } > > + state |= nsIWebProgressListener::STATE_LOADED_TRACKING_CONTENT; > > + eventSink->OnSecurityChange(nullptr, state); > > If we failed getting the securityUI, we may be inadvertently resetting the > state on the eventSink here, unless you have verified that all listeners > only OR in the new state (seems unlikely). We probably want to fail early if > there's no securityUI. > > @@ +329,5 @@ > > + NS_ENSURE_SUCCESS(rv, rv); > > + nsCOMPtr<nsPIDOMWindow> pwin = do_QueryInterface(win, &rv); > > + NS_ENSURE_SUCCESS(rv, rv); > > + nsCOMPtr<nsIDocShell> docShell = pwin->GetDocShell(); > > + if (!docShell) > > braces around return > > ::: netwerk/base/src/nsChannelClassifier.h > @@ +29,5 @@ > > void MarkEntryClassified(nsresult status); > > bool HasBeenClassified(nsIChannel *aChannel); > > // Whether or not tracking protection should be enabled on this channel. > > nsresult ShouldEnableTrackingProtection(nsIChannel *aChannel, bool *result); > > + // Act upon blocking tracking content: update security state of the > > Better comment is: // If we are blocking tracking content, update the > docshell with nsISecurityEventSink::onSecurityChange.

Comment on attachment 8471121 [details] [diff] [review] nsChannelClassifier will update the security state of the document and fire a security state change event to include the nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance Review of attachment 8471121 [details] [diff] [review]: ----------------------------------------------------------------- Hey Pat, This is a followup bug to bug 1033871. The test coverage is in bug 1043801, which adds the frontend notifications. Do you have time for this review? Thanks, Monica

Comment on attachment 8471121 [details] [diff] [review] nsChannelClassifier will update the security state of the document and fire a security state change event to include the nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance Review of attachment 8471121 [details] [diff] [review]: ----------------------------------------------------------------- this is pretty good.. I'd like to see some small problems addressed before r+ ::: netwerk/base/src/nsChannelClassifier.cpp @@ +15,5 @@ > #include "nsIPermissionManager.h" > #include "nsIProtocolHandler.h" > #include "nsIScriptSecurityManager.h" > > +#include "nsPIDOMWindow.h" the above set of includes is alphabetized - please integrate into that list @@ +134,5 @@ > + > + // Tracking protection will be disabled so update the security state > + // of the document and fire a secure change event. > + nsCOMPtr<nsPIDOMWindow> pwin = do_QueryInterface(win, &rv); > + NS_ENSURE_SUCCESS(rv, rv); if QI fails you return an error.. but if pwin->GetDocShell() fails you return OK. I would think you would ignore the problem and return OK in both situations. @@ +140,5 @@ > + if (!docShell) { > + return NS_OK; > + } > + nsCOMPtr<nsIDocument> doc = do_GetInterface(docShell, &rv); > + NS_ENSURE_SUCCESS(rv, rv); OK? @@ +141,5 @@ > + return NS_OK; > + } > + nsCOMPtr<nsIDocument> doc = do_GetInterface(docShell, &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + doc->SetHasTrackingContentLoaded(true); maybe you shouldn't do this until right before securityUI->GetState() so that the changes are atomic with respect to all of these early returns (some of which are already OK) @@ +146,5 @@ > + > + // Notify nsIWebProgressListeners of this security event. > + // Can be used to change the UI state. > + nsCOMPtr<nsISecurityEventSink> eventSink = do_QueryInterface(docShell, &rv); > + NS_ENSURE_SUCCESS(rv, rv); ok? @@ +319,5 @@ > return tag.EqualsLiteral("1"); > } > > +nsresult > +nsChannelClassifier::SetBlockedTrackingContent(nsIChannel* channel) nsichannel[space]*channel @@ +325,5 @@ > + nsresult rv; > + nsCOMPtr<nsIDOMWindow> win; > + nsCOMPtr<mozIThirdPartyUtil> thirdPartyUtil = > + do_GetService(THIRDPARTYUTIL_CONTRACTID, &rv); > + NS_ENSURE_SUCCESS(rv, rv); this function also contains a mixture of early returns that do NS_OK and early returns that return an error, and doc->SetBlah() that happens before some of the early returns. That can be easily cleaned up. ::: netwerk/base/src/nsChannelClassifier.h @@ +31,5 @@ > // Whether or not tracking protection should be enabled on this channel. > nsresult ShouldEnableTrackingProtection(nsIChannel *aChannel, bool *result); > + // If we are blocking tracking content, update the corresponding flag in > + // the respective docshell and call nsISecurityEventSink::onSecurityChange. > + nsresult SetBlockedTrackingContent(nsIChannel* channel); nsIChannel[space]*channel

Comment on attachment 8471782 [details] [diff] [review] nsChannelClassifier will update the security state of the document and fire a security state change event to include the nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance Review of attachment 8471782 [details] [diff] [review]: ----------------------------------------------------------------- Previous review comments addressed

Comment on attachment 8471782 [details] [diff] [review] nsChannelClassifier will update the security state of the document and fire a security state change event to include the nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance Review of attachment 8471782 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsChannelClassifier.cpp @@ +318,5 @@ > return tag.EqualsLiteral("1"); > } > > +nsresult > +nsChannelClassifier::SetBlockedTrackingContent(nsIChannel* channel) nsIChannel *channel please (space star) ::: netwerk/base/src/nsChannelClassifier.h @@ +31,5 @@ > // Whether or not tracking protection should be enabled on this channel. > nsresult ShouldEnableTrackingProtection(nsIChannel *aChannel, bool *result); > + // If we are blocking tracking content, update the corresponding flag in > + // the respective docshell and call nsISecurityEventSink::onSecurityChange. > + nsresult SetBlockedTrackingContent(nsIChannel* channel); nsIChannel *channel please (space star)

(In reply to Patrick McManus [:mcmanus] from comment #24) > Comment on attachment 8471782 [details] [diff] [review] > nsChannelClassifier will update the security state of the document and fire > a security state change event to include the > nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or > STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance > > Review of attachment 8471782 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/src/nsChannelClassifier.cpp > @@ +318,5 @@ > > return tag.EqualsLiteral("1"); > > } > > > > +nsresult > > +nsChannelClassifier::SetBlockedTrackingContent(nsIChannel* channel) > > nsIChannel *channel please (space star) > > ::: netwerk/base/src/nsChannelClassifier.h > @@ +31,5 @@ > > // Whether or not tracking protection should be enabled on this channel. > > nsresult ShouldEnableTrackingProtection(nsIChannel *aChannel, bool *result); > > + // If we are blocking tracking content, update the corresponding flag in > > + // the respective docshell and call nsISecurityEventSink::onSecurityChange. > > + nsresult SetBlockedTrackingContent(nsIChannel* channel); > > nsIChannel *channel please (space star) I was so sure I'd fixed that :)

Comment on attachment 8471841 [details] [diff] [review] nsChannelClassifier will update the security state of the document and fire a security state change event to include the nsIWebProgressListener::STATE_BLOCKED_TRACKING_CONTENT or STATE_LOADED_TRACKING_CONTENT flag depending on whether it is about to cance Carrying over r+ Thanks, Pat! https://hg.mozilla.org/integration/mozilla-inbound/rev/4c995a0d9166