From f7d98d769045c7aa8231692c4a833fde579214f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Wed, 14 Jun 2023 00:55:08 +0200 Subject: [PATCH] Redo wrapper for localStorage integration I think the issues in T329299 are caused by partially autosaved comments. We store data in multiple localStorage keys, and if some of them are stored but others are not (due to exceeding storage quota), our code can't handle the inconsistent state. We already have a wrapper around localStorage that tries to cover up these issues. Change it so that all values specific to an instance of a reply tool are stored under one localStorage key. This ensures that all updates consistently succeed or fail, with no partially stored state. One of the reasons we haven't done this is because this requires the whole data to be serialized to JSON every time, but our experience with VE change 4355d697aa shows that this is fast enough. Extra changes: * Remove storagePrefix, now redundant * Remove use of createConflictableStorage, now redundant * Prefix the key with 'mw' as advised by mw.storage documentation * Use ES6 syntax for the new code (just for fun) * Use consistent expiry (T339042) Bug: T329299 Change-Id: I347115f7187fd7d6afd9c6f368441e262154233b --- extension.json | 2 +- modules/.eslintrc.json | 3 +- modules/CommentController.js | 6 ++- modules/MemoryStorage.js | 75 +++++++++++++++++++++++++++ modules/NewTopicController.js | 17 +++--- modules/controller.js | 34 ++++++------ modules/createMemoryStorage.js | 83 ------------------------------ modules/dt.ui.ReplyWidget.js | 27 ++++------ modules/dt.ui.ReplyWidgetPlain.js | 6 +-- modules/dt.ui.ReplyWidgetVisual.js | 13 ++--- 10 files changed, 125 insertions(+), 141 deletions(-) create mode 100644 modules/MemoryStorage.js delete mode 100644 modules/createMemoryStorage.js diff --git a/extension.json b/extension.json index 131a7228f..79f5043c4 100644 --- a/extension.json +++ b/extension.json @@ -78,7 +78,7 @@ "CommentItem.js", "HeadingItem.js", "CommentDetails.js", - "createMemoryStorage.js", + "MemoryStorage.js", "lib/moment-timezone/moment-timezone-with-data-1970-2030.js", { "name": "parser/data.json", diff --git a/modules/.eslintrc.json b/modules/.eslintrc.json index de76e4868..fb8a887aa 100644 --- a/modules/.eslintrc.json +++ b/modules/.eslintrc.json @@ -1,7 +1,7 @@ { "root": true, "extends": [ - "wikimedia/client-es5", + "wikimedia/client", "wikimedia/jquery", "wikimedia/mediawiki" ], @@ -11,6 +11,7 @@ }, "rules": { "no-implicit-globals": "off", + "no-var": "off", "max-len": "off" }, "settings": { diff --git a/modules/CommentController.js b/modules/CommentController.js index 3f928f913..e0bd4d31f 100644 --- a/modules/CommentController.js +++ b/modules/CommentController.js @@ -16,8 +16,9 @@ var * @param {jQuery} $pageContainer Page container * @param {ThreadItem} threadItem Thread item to attach new comment to * @param {ThreadItemSet} threadItemSet + * @param {MemoryStorage} storage Storage object for autosave */ -function CommentController( $pageContainer, threadItem, threadItemSet ) { +function CommentController( $pageContainer, threadItem, threadItemSet, storage ) { // Mixin constructors OO.EventEmitter.call( this ); @@ -25,6 +26,7 @@ function CommentController( $pageContainer, threadItem, threadItemSet ) { this.$pageContainer = $pageContainer; this.threadItem = threadItem; this.threadItemSet = threadItemSet; + this.storage = storage; this.newListItem = null; this.replyWidgetPromise = null; this.newComments = []; @@ -334,7 +336,7 @@ CommentController.prototype.setupReplyWidget = function ( replyWidget, data, sup CommentController.prototype.storeEditSummary = function () { if ( this.replyWidget ) { - this.replyWidget.storage.set( this.replyWidget.storagePrefix + '/summary', this.replyWidget.getEditSummary() ); + this.replyWidget.storage.set( 'summary', this.replyWidget.getEditSummary() ); } }; diff --git a/modules/MemoryStorage.js b/modules/MemoryStorage.js new file mode 100644 index 000000000..ae3bfb475 --- /dev/null +++ b/modules/MemoryStorage.js @@ -0,0 +1,75 @@ +/** + * MemoryStorage is a wrapper around mw.SafeStorage objects. + * + * It provides two mechanisms to improve their behavior when the underlying storage mechanism fails + * (e.g. quota exceeded): + * + * - Duplicating their contents in memory, so that the storage can be relied on before the page has + * been reloaded. + * - Storing all keys in a single underlying key, to make the stores are atomic: either all values + * are stored or none. + * + * This has two additional consequences which are convenient for our use case: + * + * - All keys share the same expiry time, removing the need to specify it repeatedly. + * - When multiple processes try to write the same keys, all keys are overwritten, so that we don't + * have to worry about inconsistent data. + * + * @example + * var sessionStorage = new MemoryStorage( mw.storage.session, 'myprefix' ); + * var localStorage = new MemoryStorage( mw.storage, 'myprefix' ); + */ +class MemoryStorage { + /** + * @param {mw.SafeStorage} backend + * @param {string} key Key name to store under + * @param {number} [expiry] Number of seconds after which this item can be deleted + */ + constructor( backend, key, expiry ) { + this.backend = backend; + this.key = key; + this.expiry = expiry; + + this.data = this.backend.getObject( this.key ) || {}; + } + + get( key ) { + if ( Object.prototype.hasOwnProperty.call( this.data, key ) ) { + return this.data[ key ]; + } + return null; + } + + set( key, value ) { + this.data[ key ] = value; + this.backend.setObject( this.key, this.data, this.expiry ); + return true; + } + + remove( key ) { + delete this.data[ key ]; + if ( Object.keys( this.data ).length === 0 ) { + this.backend.remove( this.key ); + } else { + this.backend.setObject( this.key, this.data, this.expiry ); + } + return true; + } + + clear() { + this.data = {}; + this.backend.remove( this.key ); + } + + // For compatibility with mw.SafeStorage API + getObject( key ) { + return this.get( key ); + } + + // For compatibility with mw.SafeStorage API + setObject( key, value ) { + return this.set( key, value ); + } +} + +module.exports = MemoryStorage; diff --git a/modules/NewTopicController.js b/modules/NewTopicController.js index 2afccdf2f..be4840337 100644 --- a/modules/NewTopicController.js +++ b/modules/NewTopicController.js @@ -8,8 +8,9 @@ var * @param {jQuery} $pageContainer Page container * @param {HeadingItem} threadItem * @param {ThreadItemSet} threadItemSet + * @param {MemoryStorage} storage Storage object for autosave */ -function NewTopicController( $pageContainer, threadItem, threadItemSet ) { +function NewTopicController( $pageContainer, threadItem, threadItemSet, storage ) { this.container = new OO.ui.PanelLayout( { classes: [ 'ext-discussiontools-ui-newTopic' ], expanded: false, @@ -39,7 +40,7 @@ function NewTopicController( $pageContainer, threadItem, threadItemSet ) { threadItem.range.endContainer = this.sectionTitleField.$element[ 0 ]; threadItem.range.endOffset = this.sectionTitleField.$element[ 0 ].childNodes.length; - NewTopicController.super.call( this, $pageContainer, threadItem, threadItemSet ); + NewTopicController.super.call( this, $pageContainer, threadItem, threadItemSet, storage ); } OO.inheritClass( NewTopicController, CommentController ); @@ -120,7 +121,7 @@ NewTopicController.prototype.setupReplyWidget = function ( replyWidget, data ) { } mw.hook( 'wikipage.content' ).fire( this.$notices ); - var title = this.replyWidget.storage.get( this.replyWidget.storagePrefix + '/title' ); + var title = this.replyWidget.storage.get( 'title' ); if ( title && !this.sectionTitle.getValue() ) { // Don't overwrite if the user has already typed something in while the widget was loading. // TODO This should happen immediately rather than waiting for the reply widget to load, @@ -128,12 +129,12 @@ NewTopicController.prototype.setupReplyWidget = function ( replyWidget, data ) { this.sectionTitle.setValue( title ); this.prevTitleText = title; - if ( this.replyWidget.storage.get( this.replyWidget.storagePrefix + '/summary' ) === null ) { + if ( this.replyWidget.storage.get( 'summary' ) === null ) { var generatedSummary = this.generateSummary( title ); this.replyWidget.editSummaryInput.setValue( generatedSummary ); } } - this.replyWidget.storage.set( this.replyWidget.storagePrefix + '/title', this.sectionTitle.getValue() ); + this.replyWidget.storage.set( 'title', this.sectionTitle.getValue() ); if ( this.replyWidget.modeTabSelect ) { // Start with the mode-select widget not-tabbable so focus will go from the title to the body @@ -173,7 +174,7 @@ NewTopicController.prototype.clear = function () { NewTopicController.prototype.clearStorage = function () { // This is going to get called as part of the teardown chain from replywidget if ( this.replyWidget ) { - this.replyWidget.storage.remove( this.replyWidget.storagePrefix + '/title' ); + this.replyWidget.storage.remove( 'title' ); } }; @@ -183,7 +184,7 @@ NewTopicController.prototype.storeEditSummary = function () { var generatedSummary = this.generateSummary( this.sectionTitle.getValue() ); if ( currentSummary === generatedSummary ) { // Do not store generated summaries (T315730) - this.replyWidget.storage.remove( this.replyWidget.storagePrefix + '/summary' ); + this.replyWidget.storage.remove( 'summary' ); return; } } @@ -289,7 +290,7 @@ NewTopicController.prototype.onSectionTitleChange = function () { var prevTitleText = this.prevTitleText; if ( prevTitleText !== titleText ) { - this.replyWidget.storage.set( this.replyWidget.storagePrefix + '/title', titleText ); + this.replyWidget.storage.set( 'title', titleText ); var generatedSummary = this.generateSummary( titleText ); var generatedPrevSummary = this.generateSummary( prevTitleText ); diff --git a/modules/controller.js b/modules/controller.js index 0e91fb9ca..3c4c059dd 100644 --- a/modules/controller.js +++ b/modules/controller.js @@ -5,8 +5,8 @@ var pageThreads, lastControllerScrollOffset, featuresEnabled = mw.config.get( 'wgDiscussionToolsFeaturesEnabled' ) || {}, - createMemoryStorage = require( './createMemoryStorage.js' ), - storage = createMemoryStorage( mw.storage ), + MemoryStorage = require( './MemoryStorage.js' ), + STORAGE_EXPIRY = 60 * 60 * 24 * 30, Parser = require( './Parser.js' ), ThreadItemSet = require( './ThreadItemSet.js' ), CommentDetails = require( './CommentDetails.js' ), @@ -355,18 +355,22 @@ function init( $container, state ) { * @param {string} [mode] Optionally force a mode, 'visual' or 'source' * @param {boolean} [hideErrors] Suppress errors, e.g. when restoring auto-save * @param {boolean} [suppressNotifications] Don't notify the user if recovering auto-save + * @param {MemoryStorage} [storage] Storage object for autosave */ - function setupController( comment, $link, mode, hideErrors, suppressNotifications ) { + function setupController( comment, $link, mode, hideErrors, suppressNotifications, storage ) { var commentController, $addSectionLink; + if ( !storage ) { + storage = new MemoryStorage( mw.storage, 'mw-ext-DiscussionTools-reply/' + comment.id, STORAGE_EXPIRY ); + } if ( comment.id === utils.NEW_TOPIC_COMMENT_ID ) { // eslint-disable-next-line no-jquery/no-global-selector $addSectionLink = $( '#ca-addsection' ).find( 'a' ); // When opening new topic tool using any link, always activate the link in page tabs too $link = $link.add( $addSectionLink ); - commentController = new NewTopicController( $pageContainer, comment, pageThreads ); + commentController = new NewTopicController( $pageContainer, comment, pageThreads, storage ); } else { - commentController = new CommentController( $pageContainer, comment, pageThreads ); + commentController = new CommentController( $pageContainer, comment, pageThreads, storage ); } activeCommentId = comment.id; @@ -473,13 +477,14 @@ function init( $container, state ) { return; } - var mode, $link; + var mode, $link, storage; for ( var i = 0; i < pageThreads.threadItems.length; i++ ) { var comment = pageThreads.threadItems[ i ]; - if ( storage.get( 'reply/' + comment.id + '/saveable' ) ) { - mode = storage.get( 'reply/' + comment.id + '/mode' ); + storage = new MemoryStorage( mw.storage, 'mw-ext-DiscussionTools-reply/' + comment.id, STORAGE_EXPIRY ); + if ( storage.get( 'saveable' ) ) { + mode = storage.get( 'mode' ); $link = $( commentNodes[ i ] ); - setupController( comment, $link, mode, true, !state.firstLoad ); + setupController( comment, $link, mode, true, !state.firstLoad, storage ); if ( OO.ui.isMobile() ) { var urlFragment = mw.util.escapeIdForLink( comment.id ); // Force the section to expand on mobile (T338920) @@ -488,12 +493,10 @@ function init( $container, state ) { break; } } - if ( - storage.get( 'reply/' + utils.NEW_TOPIC_COMMENT_ID + '/saveable' ) || - storage.get( 'reply/' + utils.NEW_TOPIC_COMMENT_ID + '/title' ) - ) { - mode = storage.get( 'reply/' + utils.NEW_TOPIC_COMMENT_ID + '/mode' ); - setupController( newTopicComment(), $( [] ), mode, true, !state.firstLoad ); + storage = new MemoryStorage( mw.storage, 'mw-ext-DiscussionTools-reply/' + utils.NEW_TOPIC_COMMENT_ID, STORAGE_EXPIRY ); + if ( storage.get( 'saveable' ) || storage.get( 'title' ) ) { + mode = storage.get( 'mode' ); + setupController( newTopicComment(), $( [] ), mode, true, !state.firstLoad, storage ); } else if ( mw.config.get( 'wgDiscussionToolsStartNewTopicTool' ) ) { var data = linksController.parseNewTopicLink( location.href ); setupController( newTopicComment( data ), $( [] ) ); @@ -765,7 +768,6 @@ module.exports = { checkThreadItemOnPage: checkThreadItemOnPage, getCheckboxesPromise: getCheckboxesPromise, getApi: getApi, - storage: storage, getReplyWidgetModules: getReplyWidgetModules, defaultVisual: defaultVisual, enable2017Wikitext: enable2017Wikitext diff --git a/modules/createMemoryStorage.js b/modules/createMemoryStorage.js deleted file mode 100644 index bb5874dee..000000000 --- a/modules/createMemoryStorage.js +++ /dev/null @@ -1,83 +0,0 @@ -/** - * createMemoryStorage creates a wrapper around mw.SafeStorage objects, duplicating - * their contents in memory, so that even if the underlying storage mechanism - * fails (e.g. quota exceeded), the storage can be relied on before the - * page has been reloaded. - * - * @example - * var sessionStorage = createMemoryStorage( mw.storage.session ); - * var localStorage = createMemoryStorage( mw.storage ); - * - * @param {mw.SafeStorage} storage - * @return {MemoryStorage} - */ -var createMemoryStorage = function ( storage ) { - - /** - * @class - * @extends mw.SafeStorage - * - * @constructor - * @param {Storage|undefined} store The Storage instance to wrap around - */ - function MemoryStorage( store ) { - this.data = {}; - - // Attempt to populate memory cache with existing data. - // Ignore any errors accessing native Storage object, as in mw.SafeStorage. - try { - for ( var i = 0, l = store.length; i < l; i++ ) { - var key = store.key( i ); - this.data[ key ] = store.getItem( key ); - } - } catch ( e ) {} - - // Parent constructor - MemoryStorage.super.apply( this, arguments ); - } - - /* Inheritance */ - - var ParentStorage = storage.constructor; - OO.inheritClass( MemoryStorage, ParentStorage ); - - /* Methods */ - - /** - * @inheritdoc - */ - MemoryStorage.prototype.get = function ( key ) { - if ( Object.prototype.hasOwnProperty.call( this.data, key ) ) { - return this.data[ key ]; - } else { - // Parent method - return MemoryStorage.super.prototype.get.apply( this, arguments ); - } - }; - - /** - * @inheritdoc - */ - MemoryStorage.prototype.set = function ( key, value ) { - // Parent method - MemoryStorage.super.prototype.set.apply( this, arguments ); - - this.data[ key ] = value; - return true; - }; - - /** - * @inheritdoc - */ - MemoryStorage.prototype.remove = function ( key ) { - // Parent method - MemoryStorage.super.prototype.remove.apply( this, arguments ); - - delete this.data[ key ]; - return true; - }; - - return new MemoryStorage( storage.store ); -}; - -module.exports = createMemoryStorage; diff --git a/modules/dt.ui.ReplyWidget.js b/modules/dt.ui.ReplyWidget.js index 6419ad2bf..3cecbf959 100644 --- a/modules/dt.ui.ReplyWidget.js +++ b/modules/dt.ui.ReplyWidget.js @@ -43,9 +43,7 @@ function ReplyWidget( commentController, commentDetails, config ) { this.pageExists = mw.config.get( 'wgRelevantArticleId', 0 ) !== 0; var contextNode = utils.closestElement( threadItem.range.endContainer, [ 'dl', 'ul', 'ol' ] ); this.context = contextNode ? contextNode.tagName.toLowerCase() : 'dl'; - // TODO: Should storagePrefix include pageName? - this.storagePrefix = 'reply/' + threadItem.id; - this.storage = controller.storage; + this.storage = commentController.storage; // eslint-disable-next-line no-jquery/no-global-selector this.contentDir = $( '#mw-content-text' ).css( 'direction' ); this.hideNewCommentsWarning = false; @@ -390,7 +388,7 @@ ReplyWidget.prototype.clear = function ( preserveStorage ) { if ( !preserveStorage ) { this.clearStorage(); } else { - this.storage.set( this.storagePrefix + '/saveable', '1' ); + this.storage.set( 'saveable', '1' ); } this.emit( 'clear' ); @@ -400,12 +398,7 @@ ReplyWidget.prototype.clear = function ( preserveStorage ) { * Remove any storage that the widget is using */ ReplyWidget.prototype.clearStorage = function () { - this.storage.remove( this.storagePrefix + '/mode' ); - this.storage.remove( this.storagePrefix + '/saveable' ); - this.storage.remove( this.storagePrefix + '/summary' ); - this.storage.remove( this.storagePrefix + '/showAdvanced' ); - this.storage.remove( this.storagePrefix + '/formToken' ); - + this.storage.clear(); this.emit( 'clearStorage' ); }; @@ -498,7 +491,7 @@ ReplyWidget.prototype.toggleAdvanced = function ( showAdvanced ) { this.advanced.toggle( !!this.showAdvanced ); this.advancedToggle.setIndicator( this.showAdvanced ? 'up' : 'down' ); - this.storage.set( this.storagePrefix + '/showAdvanced', this.showAdvanced ? '1' : '' ); + this.storage.set( 'showAdvanced', this.showAdvanced ? '1' : '' ); }; ReplyWidget.prototype.onEditSummaryChange = function () { @@ -585,7 +578,7 @@ ReplyWidget.prototype.setup = function ( data ) { } this.saveEditMode( this.getMode() ); - var summary = this.storage.get( this.storagePrefix + '/summary' ) || data.editSummary; + var summary = this.storage.get( 'summary' ) || data.editSummary; if ( !summary ) { if ( this.isNewTopic ) { @@ -600,7 +593,7 @@ ReplyWidget.prototype.setup = function ( data ) { } this.toggleAdvanced( - !!this.storage.get( this.storagePrefix + '/showAdvanced' ) || + !!this.storage.get( 'showAdvanced' ) || !!+mw.user.options.get( 'discussiontools-showadvanced' ) || !!data.showAdvanced ); @@ -631,7 +624,7 @@ ReplyWidget.prototype.afterSetup = function () { // Init preview and button state this.onInputChange(); // Autosave - this.storage.set( this.storagePrefix + '/mode', this.getMode() ); + this.storage.set( 'mode', this.getMode() ); }; /** @@ -640,12 +633,12 @@ ReplyWidget.prototype.afterSetup = function () { * @return {string} Form token */ ReplyWidget.prototype.getFormToken = function () { - var formToken = this.storage.get( this.storagePrefix + '/formToken' ); + var formToken = this.storage.get( 'formToken' ); if ( !formToken ) { // See ApiBase::PARAM_MAX_CHARS in ApiDiscussionToolsEdit.php var maxLength = 16; formToken = Math.random().toString( 36 ).slice( 2, maxLength + 2 ); - this.storage.set( this.storagePrefix + '/formToken', formToken ); + this.storage.set( 'formToken', formToken ); } return formToken; }; @@ -771,7 +764,7 @@ ReplyWidget.prototype.onInputChange = function () { } this.updateButtons(); - this.storage.set( this.storagePrefix + '/saveable', this.isEmpty() ? '' : '1' ); + this.storage.set( 'saveable', this.isEmpty() ? '' : '1' ); this.preparePreview(); }; diff --git a/modules/dt.ui.ReplyWidgetPlain.js b/modules/dt.ui.ReplyWidgetPlain.js index 1ac7cd24d..f1a7235d7 100644 --- a/modules/dt.ui.ReplyWidgetPlain.js +++ b/modules/dt.ui.ReplyWidgetPlain.js @@ -92,7 +92,7 @@ ReplyWidgetPlain.prototype.clear = function ( preserveStorage ) { this.replyBodyWidget.setValue( '' ); if ( !preserveStorage ) { - this.storage.remove( this.storagePrefix + '/body' ); + this.storage.remove( 'body' ); } // Parent method @@ -126,14 +126,14 @@ ReplyWidgetPlain.prototype.onInputChange = function () { ReplyWidgetPlain.super.prototype.onInputChange.apply( this, arguments ); var wikitext = this.getValue(); - this.storage.set( this.storagePrefix + '/body', wikitext ); + this.storage.set( 'body', wikitext ); }; /** * @inheritdoc */ ReplyWidgetPlain.prototype.setup = function ( data ) { - var autosaveValue = this.storage.get( this.storagePrefix + '/body' ); + var autosaveValue = this.storage.get( 'body' ); data = data || {}; diff --git a/modules/dt.ui.ReplyWidgetVisual.js b/modules/dt.ui.ReplyWidgetVisual.js index 41f777538..7ff3737a1 100644 --- a/modules/dt.ui.ReplyWidgetVisual.js +++ b/modules/dt.ui.ReplyWidgetVisual.js @@ -96,8 +96,8 @@ ReplyWidgetVisual.prototype.setup = function ( data, suppressNotifications ) { data = data || {}; var htmlOrDoc; - if ( this.storage.get( this.storagePrefix + '/saveable' ) ) { - htmlOrDoc = this.storage.get( this.storagePrefix + '/ve-dochtml' ); + if ( this.storage.get( 'saveable' ) ) { + htmlOrDoc = this.storage.get( 've-dochtml' ); target.recovered = true; } else { htmlOrDoc = data.value; @@ -115,16 +115,9 @@ ReplyWidgetVisual.prototype.setup = function ( data, suppressNotifications ) { focus: [ 'emit', 'bodyFocus' ] } ); - var listStorage = ve.init.platform.createConflictableStorage( widget.storage ); - // widget.storage is a MemoryStorage object. Copy over the .data cache so - // that listStorage reads from/writes to the same in-memory cache. - listStorage.data = widget.storage.data; - target.initAutosave( { suppressNotifications: suppressNotifications, - docId: widget.storagePrefix, - storage: listStorage, - storageExpiry: 60 * 60 * 24 * 30 + storage: widget.storage } ); widget.afterSetup();