Merge pull request #4032 from bsclifton/ledger-dont-count-4xx

Don't register a visit if page returns a non-successful HTTP status code

js/lib/errorUtil.js → app/common/lib/httpUtil.js

@@ -392,3 +392,19 @@ module.exports.errorMap = {
  // Failed to sort addresses according to RFC3484.
  806: 'dnsSortError'
 }
+
+/**
+ * Returns true if HTTP response code is one we want to collect usage for
+ * @param {number} responseCode - HTTP response code to be evaluated
+ * @return {boolean} true if the code represents one w/ content, false if not
+ */
+module.exports.responseHasContent = (responseCode) => {
+ switch (responseCode) {
+ case 200: // ok
+ case 203: // non-authoritative
+ case 206: // partial content
+ case 304: // not modified (cached)
+ return true
+ }
+ return false
+}

app/common/lib/ledgerUtil.js

@@ -0,0 +1,41 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict'
+
+const { responseHasContent } = require('./httpUtil')
+
+/**
+ * Is page an actual page being viewed by the user? (not an error page, etc)
+ * If the page is invalid, we don't want to collect usage info.
+ * @param {Object} view - an entry from page_view (from EventStore)
+ * @param {Object} responseList - full page_response array (from EventStore)
+ * @return {boolean} true if page should have usage collected, false if not
+ */
+module.exports.shouldTrackView = (view, responseList) => {
+ if (!view || !view.url || !view.tabId) {
+ return false
+ }
+ if (!responseList || !Array.isArray(responseList) || !responseList.length) {
+ return false
+ }
+
+ const tabId = view.tabId
+ const url = view.url
+
+ for (let i = responseList.length; i > -1; i--) {
+ const response = responseList[i]
+
+ if (!response) continue
+
+ const responseUrl = response && response.details
+ ? response.details.newURL
+ : null
+
+ if (url === responseUrl && response.tabId === tabId) {
+ return responseHasContent(response.details.httpResponseCode)
+ }
+ }
+ return false
+}

app/ledger.js

@@ -51,6 +51,7 @@ const locale = require('./locale')
 const appStore = require('../js/stores/appStore')
 const eventStore = require('../js/stores/eventStore')
 const rulesolver = require('./extensions/brave/content/scripts/pageInformation.js')
+const ledgerUtil = require('./common/lib/ledgerUtil')
 
 // TBD: remove these post beta [MTR]
 const logPath = 'ledger-log.json'
@@ -254,8 +255,10 @@ underscore.keys(fileTypes).forEach((fileType) => {
 signatureMax = Math.ceil(signatureMax * 1.5)
 
 eventStore.addChangeListener(() => {
- var view = eventStore.getState().toJS().page_view
- var info = eventStore.getState().toJS().page_info
+ const eventState = eventStore.getState().toJS()
+ var view = eventState.page_view
+ var info = eventState.page_info
+ var pageLoad = eventState.page_load
 
  if ((!synopsis) || (!util.isArray(info))) return
 
@@ -339,7 +342,9 @@ eventStore.addChangeListener(() => {
  })
 
  view = underscore.last(view) || {}
- visit(view.url || 'NOOP', view.timestamp || underscore.now(), view.tabId)
+ if (ledgerUtil.shouldTrackView(view, pageLoad)) {
+ visit(view.url || 'NOOP', view.timestamp || underscore.now(), view.tabId)
+ }
 })
 
 /*

js/actions/windowActions.js

@@ -1005,6 +1005,14 @@ const windowActions = {
  frameProps,
  source
  })
+ },
+
+ gotResponseDetails: function (tabId, details) {
+ dispatch({
+ actionType: WindowConstants.WINDOW_GOT_RESPONSE_DETAILS,
+ tabId,
+ details
+ })
  }
 }
 

js/components/frame.js

@@ -24,7 +24,7 @@ const getSetting = require('../settings').getSetting
 const config = require('../constants/config')
 const settings = require('../constants/settings')
 const { aboutUrls, isSourceAboutUrl, isTargetAboutUrl, getTargetAboutUrl, getBaseUrl, isNavigatableAboutPage } = require('../lib/appUrlUtil')
-const { isFrameError } = require('../lib/errorUtil')
+const { isFrameError } = require('../../app/common/lib/httpUtil')
 const locale = require('../l10n')
 const appConfig = require('../constants/appConfig')
 const { getSiteSettingsForHostPattern } = require('../state/siteSettings')
@@ -965,7 +965,9 @@ class Frame extends ImmutableComponent {
  }))
  }
  })
-
+ this.webview.addEventListener('did-get-response-details', (details) => {
+ windowActions.gotResponseDetails(this.frame.get('tabId'), details)
+ })
  // Handle zoom using Ctrl/Cmd and the mouse wheel.
  this.webview.addEventListener('mousewheel', this.onMouseWheel.bind(this))
  }

js/constants/windowConstants.js

@@ -68,7 +68,8 @@ const windowConstants = {
  WINDOW_SET_CLEAR_BROWSING_DATA_DETAIL: _,
  WINDOW_SET_AUTOFILL_ADDRESS_DETAIL: _,
  WINDOW_SET_AUTOFILL_CREDIT_CARD_DETAIL: _,
- WINDOW_SET_BLOCKED_RUN_INSECURE_CONTENT: _
+ WINDOW_SET_BLOCKED_RUN_INSECURE_CONTENT: _,
+ WINDOW_GOT_RESPONSE_DETAILS: _
 }
 
 module.exports = mapValuesByKeys(windowConstants)

js/stores/eventStore.js

@@ -10,6 +10,7 @@ const Immutable = require('immutable')
 const WindowConstants = require('../constants/windowConstants')
 const debounce = require('../lib/debounce.js')
 const { isSourceAboutUrl } = require('../lib/appUrlUtil')
+const { responseHasContent } = require('../../app/common/lib/httpUtil')
 
 const electron = require('electron')
 const BrowserWindow = electron.BrowserWindow
@@ -89,22 +90,6 @@ const windowClosed = (windowId) => {
 // Register callback to handle all updates
 const doAction = (action) => {
  switch (action.actionType) {
- case WindowConstants.WINDOW_WEBVIEW_LOAD_END:
- // create a page view event if this is a page load on the active tabId
- if (!lastActiveTabId || action.frameProps.get('tabId') === lastActiveTabId) {
- addPageView(action.frameProps.get('location'), action.frameProps.get('tabId'))
- }
-
- if (action.isError || isSourceAboutUrl(action.frameProps.get('location'))) {
- break
- }
-
- let pageLoadEvent = Immutable.fromJS({
- timestamp: new Date().getTime(),
- url: action.frameProps.get('location')
- })
- eventState = eventState.set('page_load', eventState.get('page_load').push(pageLoadEvent))
- break
  case WindowConstants.WINDOW_SET_FOCUSED_FRAME:
  lastActiveTabId = action.frameProps.get('tabId')
  addPageView(action.frameProps.get('location'), lastActiveTabId)
@@ -128,6 +113,30 @@ const doAction = (action) => {
  // retains all past pages, not really sure that's needed... [MTR]
  eventState = eventState.set('page_info', eventState.get('page_info').push(action.pageInfo))
  break
+ case WindowConstants.WINDOW_GOT_RESPONSE_DETAILS:
+ // Only capture response for the page (not subresources, like images, JavaScript, etc)
+ if (action.details && action.details.get('resourceType') === 'mainFrame') {
+ const pageUrl = action.details.get('newURL')
+
+ // create a page view event if this is a page load on the active tabId
+ if (!lastActiveTabId || action.tabId === lastActiveTabId) {
+ addPageView(pageUrl, action.tabId)
+ }
+
+ const responseCode = action.details.get('httpResponseCode')
+ if (isSourceAboutUrl(pageUrl) || !responseHasContent(responseCode)) {
+ break
+ }
+
+ const pageLoadEvent = Immutable.fromJS({
+ timestamp: new Date().getTime(),
+ url: pageUrl,
+ tabId: action.tabId,
+ details: action.details
+ })
+ eventState = eventState.set('page_load', eventState.get('page_load').push(pageLoadEvent))
+ }
+ break
  default:
  }
 

js/stores/windowStore.js

@@ -19,7 +19,7 @@ const urlParse = require('url').parse
 const currentWindow = require('../../app/renderer/currentWindow')
 const {tabFromFrame} = require('../state/frameStateUtil')
 
-const { l10nErrorText } = require('../lib/errorUtil')
+const { l10nErrorText } = require('../../app/common/lib/httpUtil')
 const { aboutUrls, getSourceAboutUrl, isIntermediateAboutPage, navigatableTypes } = require('../lib/appUrlUtil')
 const Serializer = require('../dispatcher/serializer')
 

test/unit/lib/httpUtilTest.js

@@ -0,0 +1,40 @@
+/* global describe, it */
+const httpUtil = require('../../../app/common/lib/httpUtil')
+const assert = require('assert')
+
+require('../braveUnit')
+
+describe('httpUtil test', function () {
+ describe('responseHasContent', function () {
+ describe('expected success codes', function () {
+ it('returns true for various success responses (200, 203, 206)', function () {
+ assert.equal(httpUtil.responseHasContent(200), true)
+ assert.equal(httpUtil.responseHasContent(203), true)
+ assert.equal(httpUtil.responseHasContent(206), true)
+ })
+ it('returns true for a cached response (304)', function () {
+ assert.equal(httpUtil.responseHasContent(304), true)
+ })
+ })
+
+ describe('expected failure codes', function () {
+ it('returns false for non-content success codes (used for REST apis, etc)', function () {
+ assert.equal(httpUtil.responseHasContent(201), false) // created
+ assert.equal(httpUtil.responseHasContent(202), false) // accepted
+ })
+ it('returns false for various client error responses (400+)', function () {
+ assert.equal(httpUtil.responseHasContent(400), false) // bad request
+ assert.equal(httpUtil.responseHasContent(401), false) // unauthorized
+ assert.equal(httpUtil.responseHasContent(403), false) // forbidden
+ assert.equal(httpUtil.responseHasContent(404), false) // not found
+ })
+ it('returns false for various server side error responses (500-504)', function () {
+ assert.equal(httpUtil.responseHasContent(500), false) // internal server error
+ assert.equal(httpUtil.responseHasContent(501), false) // not implemented
+ assert.equal(httpUtil.responseHasContent(502), false) // bad gateway
+ assert.equal(httpUtil.responseHasContent(503), false) // service unavailable
+ assert.equal(httpUtil.responseHasContent(504), false) // gateway timeout
+ })
+ })
+ })
+})

test/unit/lib/ledgerUtilTest.js

@@ -0,0 +1,47 @@
+/* global describe, it */
+const ledgerUtil = require('../../../app/common/lib/ledgerUtil')
+const assert = require('assert')
+
+require('../braveUnit')
+
+describe('ledgerUtil test', function () {
+ describe('shouldTrackView', function () {
+ const validView = { tabId: 1, url: 'https://brave.com/' }
+ const validResponseList = [{ tabId: validView.tabId, details: { newURL: validView.url, httpResponseCode: 200 } }]
+ const noMatchResponseList = [{ tabId: 3, details: { newURL: 'https://not-brave.com' } }]
+ const matchButErrored = [{ tabId: validView.tabId, details: { newURL: validView.url, httpResponseCode: 404 } }]
+
+ describe('input validation', function () {
+ it('returns false if view is falsey', function () {
+ assert.equal(ledgerUtil.shouldTrackView(null, validResponseList), false)
+ })
+ it('returns false if view.url is falsey', function () {
+ assert.equal(ledgerUtil.shouldTrackView({tabId: 1}, validResponseList), false)
+ })
+ it('returns false if view.tabId is falsey', function () {
+ assert.equal(ledgerUtil.shouldTrackView({url: 'https://brave.com/'}, validResponseList), false)
+ })
+ it('returns false if responseList is falsey', function () {
+ assert.equal(ledgerUtil.shouldTrackView(validView, null), false)
+ })
+ it('returns false if responseList is not an array', function () {
+ assert.equal(ledgerUtil.shouldTrackView(validView, {}), false)
+ })
+ it('returns false if responseList is a 0 length array', function () {
+ assert.equal(ledgerUtil.shouldTrackView(validView, []), false)
+ })
+ })
+
+ describe('when finding a matching response based on tabId and url', function () {
+ it('returns false if no match found', function () {
+ assert.equal(ledgerUtil.shouldTrackView(validView, noMatchResponseList), false)
+ })
+ it('returns false if match is found BUT response code is a failure (ex: 404)', function () {
+ assert.equal(ledgerUtil.shouldTrackView(validView, matchButErrored), false)
+ })
+ it('returns true when match is found AND response code is a success (ex: 200)', function () {
+ assert.equal(ledgerUtil.shouldTrackView(validView, validResponseList), true)
+ })
+ })
+ })
+})