Page MenuHomePhabricator

Likely race condition in MWMediaDialog
Open, Needs TriagePublicBUG REPORT

Description

Identified while investigating https://phabricator.wikimedia.org/T362015

Causing a delay switching panels by using breakpoints causes a misrendering of an image detail panel.

Steps to replicate the issue (include links if applicable):

  • Enter the visual editor for an article edit
  • Using the top toolbar, select Insert>Images and media
  • Choose the "search" tab and type "cat"
  • Enter the source code viewer in the browser. Navigate to ve.ui.MWMediaDialog.prototype.switchPanels
    • On Chrome you can click into the dev tools and use cmd-P to request the file, ve.ui.MWMediaDialog.js
    • cmd-F then prototype.switchPanels is probably fastest
  • Set a breakpoint on the first line of the function body
  • Click on the image of the cat
  • Click "resume script execution"

What happens?:

  • The panel loads up two copies of the image and its details inside the element with class ve-ui-mwMediaDialog-panel-imageinfo-wrapper.
  • You will see two cat pictures instead of one. The second one is badly laid out.

What should have happened instead?:

  • There should be one cat image and one detail body

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):
Latest mediawiki branch, latest VE branch.

VisualEditor master $ git rev-parse HEAD
70c21790251ae9062b7e83b4c96eed5dcead6787
mediawiki master $ git rev-parse HEAD
146b4a632d564753fa3800610fff12665a39c6ce

Other information (browser name/version, screenshots, etc.):

  • Chrome Version 124.0.6367.119 (Official Build) (arm64)
  • Mac

Screenshot 2024-05-07 at 19.23.45.png (1×1 px, 1 MB)

Event Timeline

Oddly, hitting "F8" or closing the source code editor doesn't cause this bug whereas clicking "resume script execution" does. I'm now not sure it's not a browser bug instead.

This was indeed very confusing, but I think the cause is just how we handle click events in OOUI. We bind a global mouseup listener on mousedown (then unbind it after it fires)[1]. This allows multi-select widgets to implement drag-to-select.

So when you are debugging, the click on "resume script execution" is bubbling through to the document, and queueing up another select event, which runs out of order and doesn't clear the output, appending the info panel twice.

That said it would probably make sense to move this.$infoPanelWrapper.empty() to happen inside this.buildMediaInfoPanel() to avoid future confusion.

  1. https://github.com/wikimedia/oojs-ui/blob/cc5390ef6a2b1f327d0ee1509381f79daba62af9/src/widgets/SelectWidget.js#L244