1428106 - enqueuing an early message causes state changes, triggering warning (and unnecessary re-renders?)

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Initially, I thought this was related to the React 16 upgrade, but this is actually happening on master also.  Set browser.newtabpage.activity-stream.debug to true in about:config, and restart.  Look at the browser console.  Notice this message:

09:23:23.186 Warning: setState(...): Cannot update during an existing state transition (such as within `render` or another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved to `componentWillMount`. react-dom-dev.js:18238:9
	printWarning resource://activity-stream/vendor/react-dom-dev.js:18238:9
	warning resource://activity-stream/vendor/react-dom-dev.js:18262:9
	getInternalInstanceReadyForUpdate resource://activity-stream/vendor/react-dom-dev.js:12665:38
	enqueueSetState resource://activity-stream/vendor/react-dom-dev.js:12821:28
	[6]</ReactComponent.prototype.setState resource://activity-stream/vendor/react-dev.js:1256:3
	p/</a</p.prototype.onStateChange resource://activity-stream/vendor/react-redux.js:1:4133
	notify resource://activity-stream/vendor/react-redux.js:1:951
	J</t.prototype.notifyNestedSubs resource://activity-stream/vendor/react-redux.js:1:10552
	p/</a</p.prototype.onStateChange resource://activity-stream/vendor/react-redux.js:1:4151
	dispatch resource://activity-stream/vendor/redux.js:339:8
	messageMiddleware/</< resource://activity-stream/data/content/activity-stream.bundle.js:4271:3
	queueEarlyMessageMiddleware/</< resource://activity-stream/data/content/activity-stream.bundle.js:4318:5
	rehydrationMiddleware/</< resource://activity-stream/data/content/activity-stream.bundle.js:4276:12
	_sendBadStateEvent resource://activity-stream/data/content/activity-stream.bundle.js:1380:7
	_maybeSendBadStateEvent resource://activity-stream/data/content/activity-stream.bundle.js:1332:7
	render resource://activity-stream/data/content/activity-stream.bundle.js:1419:7
	_renderValidatedComponentWithoutOwnerOrContext/renderedElement< resource://activity-stream/vendor/react-dom-dev.js:5302:16
	measureLifeCyclePerf resource://activity-stream/vendor/react-dom-dev.js:4581:12
	_renderValidatedComponentWithoutOwnerOrContext resource://activity-stream/vendor/react-dom-dev.js:5301:25
	_renderValidatedComponent resource://activity-stream/vendor/react-dom-dev.js:5328:27
	_updateRenderedComponent resource://activity-stream/vendor/react-dom-dev.js:5252:31
	_performComponentUpdate resource://activity-stream/vendor/react-dom-dev.js:5230:5
	updateComponent resource://activity-stream/vendor/react-dom-dev.js:5151:7
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:5053:5
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:11677:5
	_updateRenderedComponent resource://activity-stream/vendor/react-dom-dev.js:5260:7
	_performComponentUpdate resource://activity-stream/vendor/react-dom-dev.js:5230:5
	updateComponent resource://activity-stream/vendor/react-dom-dev.js:5151:7
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:5053:5
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:11677:5
	_updateRenderedComponent resource://activity-stream/vendor/react-dom-dev.js:5260:7
	_performComponentUpdate resource://activity-stream/vendor/react-dom-dev.js:5230:5
	updateComponent resource://activity-stream/vendor/react-dom-dev.js:5151:7
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:5053:5
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:11677:5
	updateChildren resource://activity-stream/vendor/react-dom-dev.js:4384:9
	_reconcilerUpdateChildren resource://activity-stream/vendor/react-dom-dev.js:10455:11
	_updateChildren resource://activity-stream/vendor/react-dom-dev.js:10559:26
	updateChildren resource://activity-stream/vendor/react-dom-dev.js:10546:7
	_updateDOMChildren resource://activity-stream/vendor/react-dom-dev.js:6455:7
	updateComponent resource://activity-stream/vendor/react-dom-dev.js:6273:5
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:6235:5
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:11677:5
	_updateRenderedComponent resource://activity-stream/vendor/react-dom-dev.js:5260:7
	_performComponentUpdate resource://activity-stream/vendor/react-dom-dev.js:5230:5
	updateComponent resource://activity-stream/vendor/react-dom-dev.js:5151:7
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:5053:5
	receiveComponent resource://activity-stream/vendor/react-dom-dev.js:11677:5
	_updateRenderedComponent resource://activity-stream/vendor/react-dom-dev.js:5260:7
	_performComponentUpdate resource://activity-stream/vendor/react-dom-dev.js:5230:5
	updateComponent resource://activity-stream/vendor/react-dom-dev.js:5151:7
	performUpdateIfNecessary resource://activity-stream/vendor/react-dom-dev.js:5067:7
	performUpdateIfNecessary resource://activity-stream/vendor/react-dom-dev.js:11709:5
	runBatchedUpdates resource://activity-stream/vendor/react-dom-dev.js:12997:5
	perform resource://activity-stream/vendor/react-dom-dev.js:14816:13
	perform resource://activity-stream/vendor/react-dom-dev.js:14816:13
	perform resource://activity-stream/vendor/react-dom-dev.js:12936:12
	flushBatchedUpdates resource://activity-stream/vendor/react-dom-dev.js:13019:7
	closeAll resource://activity-stream/vendor/react-dom-dev.js:14882:11
	perform resource://activity-stream/vendor/react-dom-dev.js:14829:11
	batchedUpdates resource://activity-stream/vendor/react-dom-dev.js:8862:14
	enqueueUpdate resource://activity-stream/vendor/react-dom-dev.js:13047:5
	enqueueUpdate resource://activity-stream/vendor/react-dom-dev.js:12635:3
	enqueueSetState resource://activity-stream/vendor/react-dom-dev.js:12830:5
	[6]</ReactComponent.prototype.setState resource://activity-stream/vendor/react-dev.js:1256:3
	p/</a</p.prototype.onStateChange resource://activity-stream/vendor/react-redux.js:1:4133
	notify resource://activity-stream/vendor/react-redux.js:1:951
	J</t.prototype.notifyNestedSubs resource://activity-stream/vendor/react-redux.js:1:10552
	p/</a</p.prototype.onStateChange resource://activity-stream/vendor/react-redux.js:1:4151
	dispatch resource://activity-stream/vendor/redux.js:339:8
	messageMiddleware/</< resource://activity-stream/data/content/activity-stream.bundle.js:4271:3
	queueEarlyMessageMiddleware/</< resource://activity-stream/data/content/activity-stream.bundle.js:4318:5
	rehydrationMiddleware/</< resource://activity-stream/data/content/activity-stream.bundle.js:4276:12
	initStore/< resource://activity-stream/data/content/activity-stream.bundle.js:4354:9

Ed and I spent a bunch of time poking at this yesterday.  What's happening here is that enqueueEarlyMessageMiddleware keeps its queue as part of the redux store, and we're trying to send a message to telemetry from ComponentPerfTimer.render() that render was reached without actually having the data to do the render (i.e. .initialized was not set).

Now we are doing something that makes render not be a pure function, and the sending could (and perhaps should) be moved to componentDidUpdate to avoid that.  That said, the code is guarded so that it can't actually send more than once, so it isn't really a problem, though it may be a bit fragile.

However, Ed also realized that since the queue is part of the redux state, changing it is presumably causing a re-render, which is effectively irrelevant, since _this_ particular bit of state isn't used at all in determining _what_ gets rendered.  Whether this has a measurable performance cost is unclear, but it is happening at startup, when we're quite sensitive about this sort of thing (although, to be fair, we're seeing the warning in debug mode, so that _could_ be a red herring).

At the very least, we should make the warning go away, and maybe the re-rendering is also important (and fixing that by not using the store for the queue would make the warning go away).

Kate, what are your thoughts here?

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Note that (probably separate from this), ComponentPerfTimer is likely to want to be broken up into two separate components (one for timing, one for data readiness telemetry) - (_maybe_ sharing a superclass) for easier reasoning.

Comment 2

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3961

Comment 3

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/32440259bb2c3e53e93c5b36160be9f7df7e77c1
Bug 1428106 - Add skipLocal to ComponentPerfTimer
This fix is to prevent redux from triggering additional renders

https://github.com/mozilla/activity-stream/commit/1ca429bd8d9228b58bae2a06c40f03dbebc2547a
Bug 1428106 - Rename routed messages to better reflect their behaviour
SendToMain becomes AlsoToMain
SendToMain with skipLocal=true becomes OnlyToMain
SendToContent becomes AlsoToOneContent
SendToContent with skipMain=true becomes OnlyToOneContent
SendToPreloaded becomes AlsoToPreloaded
BroadcastToContent stays as is

https://github.com/mozilla/activity-stream/commit/f6f0f36a41bbcb1b6932bfb4ab002a3299a08e67
Merge pull request #3961 from k88hudson/bug1428106

Bug 1428106 - Add skipLocal to ComponentPerfTimer

Comment 4

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/3ef8041b6eb9