Page MenuHomePhabricator

Memory-protection for sessionStorage not working
Closed, ResolvedPublic

Description

For the new-comment-notification feature we decided to put sessionStorage in a MemoryStarage wrapper, so that even sessionStorage was disabled, your reply would be recovered if you clicked the notification (as it doesn't reload the page), however the MemoryStorage was not passed to the VE surface, so this fails.

Note that the MemoryStorage wrapper was designed to protect against an edge case of someone using the new-comment-notification with sessionStorage disabled or broken, which is likely a very rare edge case, so it is unlikely anyone has been affected by this bug yet.

Steps:

Expected:

  • Your draft reply is recovered

Actual:

  • It gets stuck on "loading"

Event Timeline

Change 804757 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/DiscussionTools@master] ReplyWidgetVisual: Overwite ve.init.platform.sessionStorage with memory-wrapped store

https://gerrit.wikimedia.org/r/804757

Change 804757 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] ReplyWidgetVisual: Pass in memory-wrapped store to VE

https://gerrit.wikimedia.org/r/804757

@matmarex , I somehow thought you were trying to reopen the ticket and decided to check it.

Here's what I found: https://photos.app.goo.gl/CPQXHazFbNTtZrS8A.
On click of the the "Show 7 new comments" button:

  • Reply buttons change to [reply] link
  • Reply tool is not displayed anywhere and the esc button doesn't help. It leaves the user confused about how to interact with the page

When I reload, the Reply links are disabled and the one I initially clicked is bold and throws an error when clicked. See

Screenshot 2022-08-06 at 00.00.50.png (1×3 px, 725 KB)

@matmarex , I somehow thought you were trying to reopen the ticket and decided to check it.

Here's what I found: https://photos.app.goo.gl/CPQXHazFbNTtZrS8A.
On click of the the "Show 7 new comments" button:

  • Reply buttons change to [reply] link
  • Reply tool is not displayed anywhere and the esc button doesn't help. It leaves the user confused about how to interact with the page

When I reload, the Reply links are disabled and the one I initially clicked is bold and throws an error when clicked. See F35402184

Sorry it took me so long to get back to this. These issues don't seem to be related to the changes in this task.

Considering that the Reply buttons changed to [reply] links, I suspect that the issues you ran into were related to the rollout of that feature (T255560) – specifically, it looks like when you viewed the page, you got the new buttons while viewing the (uncached) old revision, and then old links while loading the (cached) new revision. I'm not sure why the exceptions occurred in this scenario, but I don't think it's worth investigating now, when the old cached data is gone.

It looks like the errors were real and some users were actually affected by them (although very few). They seem to have stopped occurring now, so either we fixed them in another patch, or they were fixed by the old caches expiring.


image.png (2×3 px, 1 MB)

Logstash query: "Widget not found" AND "setupController"
image.png (527×2 px, 44 KB)


Screenshot 2022-08-06 at 00.00.50.png (1×3 px, 725 KB)

Logstash query: "Cannot read properties of undefined (reading 'scrollElementIntoView')" AND "CommentController.js.CommentController.showAndFocus"
image.png (527×2 px, 43 KB)