From 62cf34b58cb554324fda7cb11e130c353d954ca6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 8 Jan 2016 12:03:45 +0000 Subject: [PATCH 1/2] Fix some races due to promises completing after we've switched rooms Add a few isMounted() checks to promise handlers so that we don't end up throwing NPEs. --- src/components/structures/RoomView.js | 13 ++++++++----- src/components/structures/ScrollPanel.js | 10 ++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 9ab99d9cba..c91be22a47 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -353,11 +353,14 @@ module.exports = React.createClass({ _paginateCompleted: function() { debuglog("paginate complete"); - this.setState({ - room: MatrixClientPeg.get().getRoom(this.props.roomId) - }); + // we might have switched rooms since the paginate started - just bin + // the results if so. + if (!this.isMounted()) return; - this.setState({paginating: false}); + this.setState({ + room: MatrixClientPeg.get().getRoom(this.props.roomId), + paginating: false, + }); }, onSearchResultsFillRequest: function(backwards) { @@ -535,7 +538,7 @@ module.exports = React.createClass({ return searchPromise.then(function(results) { debuglog("search complete"); - if (!self.state.searching || self.searchId != localSearchId) { + if (!this.isMounted() || !self.state.searching || self.searchId != localSearchId) { console.error("Discarding stale search results"); return; } diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 042458717d..fc628b5a5e 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -158,6 +158,10 @@ module.exports = React.createClass({ // check the scroll state and send out backfill requests if necessary. checkFillState: function() { + if (!this.isMounted()) { + return; + } + var sn = this._getScrollNode(); // if there is less than a screenful of messages above or below the @@ -346,6 +350,12 @@ module.exports = React.createClass({ * message panel. */ _getScrollNode: function() { + if (!this.isMounted()) { + // this shouldn't happen, but when it does, turn the NPE into + // something more meaningful. + throw new Error("ScrollPanel._getScrollNode called when unmounted"); + } + var panel = ReactDOM.findDOMNode(this.refs.geminiPanel); // If the gemini scrollbar is doing its thing, this will be a div within From c30aeac31530e093adde022cdc658525a5021b21 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 11 Jan 2016 11:38:04 +0000 Subject: [PATCH 2/2] Set our own booleans instead of using isMounted --- src/components/structures/RoomView.js | 12 +++++++++--- src/components/structures/ScrollPanel.js | 12 ++++++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index c91be22a47..32deec8a9b 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -100,6 +100,12 @@ module.exports = React.createClass({ }, componentWillUnmount: function() { + // set a boolean to say we've been unmounted, which any pending + // promises can use to throw away their results. + // + // (We could use isMounted, but facebook have deprecated that.) + this.unmounted = true; + if (this.refs.messagePanel) { // disconnect the D&D event listeners from the message panel. This // is really just for hygiene - the messagePanel is going to be @@ -196,7 +202,7 @@ module.exports = React.createClass({ },*/ onRoomTimeline: function(ev, room, toStartOfTimeline) { - if (!this.isMounted()) return; + if (this.unmounted) return; // ignore anything that comes in whilst paginating: we get one // event for each new matrix event so this would cause a huge @@ -355,7 +361,7 @@ module.exports = React.createClass({ // we might have switched rooms since the paginate started - just bin // the results if so. - if (!this.isMounted()) return; + if (this.unmounted) return; this.setState({ room: MatrixClientPeg.get().getRoom(this.props.roomId), @@ -538,7 +544,7 @@ module.exports = React.createClass({ return searchPromise.then(function(results) { debuglog("search complete"); - if (!this.isMounted() || !self.state.searching || self.searchId != localSearchId) { + if (self.unmounted || !self.state.searching || self.searchId != localSearchId) { console.error("Discarding stale search results"); return; } diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index fc628b5a5e..8d26b2e365 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -112,6 +112,14 @@ module.exports = React.createClass({ this.checkFillState(); }, + componentWillUnmount: function() { + // set a boolean to say we've been unmounted, which any pending + // promises can use to throw away their results. + // + // (We could use isMounted(), but facebook have deprecated that.) + this.unmounted = true; + }, + onScroll: function(ev) { var sn = this._getScrollNode(); debuglog("Scroll event: offset now:", sn.scrollTop, "recentEventScroll:", this.recentEventScroll); @@ -158,7 +166,7 @@ module.exports = React.createClass({ // check the scroll state and send out backfill requests if necessary. checkFillState: function() { - if (!this.isMounted()) { + if (this.unmounted) { return; } @@ -350,7 +358,7 @@ module.exports = React.createClass({ * message panel. */ _getScrollNode: function() { - if (!this.isMounted()) { + if (this.unmounted) { // this shouldn't happen, but when it does, turn the NPE into // something more meaningful. throw new Error("ScrollPanel._getScrollNode called when unmounted");