Make WordPress Core

Opened 22 months ago

Closed 5 months ago

#59329 closed defect (bug) (fixed)

Firefox gets "ReferenceError: Â is not defined" in unminified moxie.js

Reported by: kinggmobb's profile kinggmobb Owned by: desrosj's profile desrosj
Milestone: 6.8 Priority: normal
Severity: minor Version: 4.9
Component: External Libraries Keywords: has-patch commit
Focuses: javascript Cc:

Description

In unminified moxie.js, there exists some weird Unicode characters on line 7391, between the BinaryReader constructor fn and the call to Basic.extend(). In a hex editor, you can see the character bytes are "C2A0", which is a random Korean Hangul character in Unicode.

In Firefox (as of the latest 117), when using Divi, this causes an exception when loading unminified moxie.js. Firefox's Js loader interprets the first byte alone as Â, and fails while loading moxie.js.

I can replicate this in FF+Divi, but the problem does not appear in Chrome, Safari, or in FF when using other builders like Classic/Gutenberg/Elementor/Beaver. Minification strips out the problem bytes.

This link, https://stackoverflow.com/a/1462039/1201409, suggests the error is leftover from an earlier era of encoding a nonbreaking space before UTF-8 replaced other encodings.

Regardless of how the bytes got in there (and whether FF/Divi have bugs), they shouldn't be there, and luckily, the fix is trivial: delete them.

Attachments (3)

Screenshot 2023-09-12 at 8.05.39 PM.png (1.1 MB) - added by kinggmobb 22 months ago.
moxie.js error in Firefox console
Screenshot 2023-09-12 at 8.07.02 PM.png (167.0 KB) - added by kinggmobb 22 months ago.
Hex editor view showing the relevant bytes
59329.diff (377 bytes) - added by kinggmobb 22 months ago.
Patch

Download all attachments as: .zip

Change History (21)

@kinggmobb
22 months ago

moxie.js error in Firefox console

@kinggmobb
22 months ago

Hex editor view showing the relevant bytes

@kinggmobb
22 months ago

Patch

This ticket was mentioned in Slack in #core by jorbin. View the logs.


20 months ago

#2 @jorbin
20 months ago

  • Keywords close added
  • Milestone Awaiting Review deleted
  • Version trunk deleted

Welcome to trac @kinggmobb and thanks for the report. While you are seeing this error in WordPress, plupload is not a library that WordPress maintains.

Can you report this upstream to https://github.com/moxiecode/plupload and then when there is a new release WordPress can update to that?

I'm adding the close tag so that after it is reported upstream, we can close this as reported-upstream

#3 @kinggmobb
20 months ago

I was prepping to file a bug upstream, but then I took a closer look at the plupload and moxie repos.

I ran a git bisect, and turns out, the bug was silently introduced, and then inadvertently fixed, 7 years ago. WordPress is just using an ancient version of moxie.js from that time window. (There's a reference to the message I saw in https://github.com/moxiecode/moxie/issues/144, but I don't think anyone realized the error at the time.)

The issue was fixed right before moxie v1.5.0, in commit b9f329e1add1e19752263fe62a6b7d993bbd1906.

I think if WP upgrades to a moxie version from v1.5.0 on, it should fix the problem. Alternatively, my patch is trivial if you don't feel like upgrading moxie.js.

#4 @desrosj
11 months ago

  • Keywords has-patch needs-testing added; close removed
  • Milestone set to 6.7

Related #48277, #40158, #41755.

To my understanding, the AGPL is not compatible with the GPL, so simply updating Moxie and Plupload to their latest versions is not a path that can be pursued.

59329.diff seems like a simple and reasonable change to make to our now "adopted" version of the library.

@jorbin do you have any objection?

#5 @q0rban
9 months ago

I'm running into this same error in FireFox, when viewing site-editor.php.

#6 @desrosj
8 months ago

  • Milestone changed from 6.7 to 6.7.1

Apologies that this did not make it into 6.7. RC1 is due out any moment, so I'm going to punt to 6.7.1.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


7 months ago

#8 @desrosj
7 months ago

  • Milestone changed from 6.7.1 to 6.7.2
  • Version set to 4.9

I did some digging, and it seems like this may have been introduced around the time of #41755. Updating the Version accordingly.

Because of a few bug reports opened since 6.7 was released, the Core team is evaluating the need for a short 6.7.1 cycle (possibly next week).

This issue was not introduced in 6.7, so it now falls outside of the focus for 6.7.1 as currently defined. To help prepare for this scenario in case it's decided to move forward, I'm going to punt this to the 6.7.2 milestone.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 months ago

#10 follow-up: @desrosj
6 months ago

  • Keywords changes-requested added; needs-testing removed
  • Milestone changed from 6.7.2 to 6.8

After discussing this in today's bug scrub, it's ready to commit, but going to target 6.8 instead.

59329.diff is good to be committed. However, the version for moxie as defined in script-loader.php needs to be changed to ensure all caches are busted.

We could go few directions here:

  • Simply add -wp to the end of the current 1.3.5 version
  • Since this is now an adopted fork due to upstream license changes, change the version to avoid confusion with the original upstream library. The latest is 3.1.5, so not sure that this path is realistic.
  • A combination of the two. Maybe user 1.3.6-wp.

Since this is a very simple change, I think I lean towards option 1.

#11 in reply to: ↑ 10 ; follow-up: @azaozz
6 months ago

Replying to desrosj:

Simply add -wp to the end of the current 1.3.5 version
...
A combination of the two. Maybe user 1.3.6-wp.

Think I remember having some problems with plugins when the version string of an external JS library was changed in a similar way (the plugin was expecting only digits and dots?). As this is a minor release perhaps better to "play it safe" and go with 1.3.5.1 perhaps? Also may be good to add another header to the file explaining that this file was modified/is a (de-facto) fork.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


6 months ago

#13 in reply to: ↑ 11 ; follow-up: @desrosj
6 months ago

Replying to azaozz:

Think I remember having some problems with plugins when the version string of an external JS library was changed in a similar way (the plugin was expecting only digits and dots?). As this is a minor release perhaps better to "play it safe" and go with 1.3.5.1 perhaps? Also may be good to add another header to the file explaining that this file was modified/is a (de-facto) fork.

Ah! Yes, I like that better. Let's go with 1.3.5.1 and introduce a header. plupload.js already has one, but doesn't explicitly say that the file is a de-facto fork.

I looked at some libraries in a similar situation (both PHP and JS), but couldn't find one with a header to that effect. It may be worth standardizing this and adding to all of our adopted libraries.

#14 in reply to: ↑ 13 @azaozz
6 months ago

Replying to desrosj:

plupload.js already has one, but doesn't explicitly say that the file is a de-facto fork.

I looked at some libraries in a similar situation (both PHP and JS), but couldn't find one with a header to that effect.

Ah, I meant adding a note to the headers that the file was modified for WP. Sorry didn't explain it well. But probably better to explain a little more and/or mention that it is forked and will be maintained by the WP developers.

It may be worth standardizing this and adding to all of our adopted libraries.

Yes, good idea! Seems some of the de-facto forked libs/files like twemoji.js have some inline comments. Others like thickbox.js don't have anything although I'm sure the original Thickbox JS was modified many times to make it work with newer jQuery, etc. (unbelievably it is from 2007, and still works?!).

#16 @desrosj
5 months ago

  • Keywords changes-requested removed
  • Owner set to desrosj
  • Status changed from new to reviewing

#17 @desrosj
5 months ago

  • Component changed from Upload to External Libraries
  • Keywords commit added

#18 @desrosj
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 59770:

External Libraries: Remove stray Unicode character in moxie.js

This fixes a ReferenceError caused by a stray Unicode character in the unminified version of moxie.js. This has long been fixed upstream but the library cannot be wholesale updated in WordPress because of an incompatible license change.

Because of this, a new version is being tagged, 1.3.5.1, and the file header has been updated to make it more clear that the file is a maintained fork with a high level list of changes made.

Props kinggmobb, jorbin, q0rban, azaozz, desrosj, sukhendu2002.
Fixes #59329.

Note: See TracTickets for help on using tickets.