Open Bug 1554014 Opened 5 years ago Updated 2 years ago

TinyMCE versions 3.4.4 and older don't work in Firefox 67

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Version:
67 Branch
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
relnote-firefox --- 67+
firefox-esr60 --- unaffected
firefox67 + wontfix
firefox67.0.1 - wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix

People

(Reporter: adv, Unassigned)

References

(Regression,
URL
)

Details

(Keywords: regression, site-compat)

Attachments

(1 file, 1 obsolete file)

!123.jpg
197.96 KB, image/jpeg
Details
Attached image !123.jpg —

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

jquery.tinymce.js
WYSIWYG HTML Editor | TinyMCE
not working in 67 Version

Actual results:

script not working
the text in the window WYSIWYG HTML Editor is not edited
no text is displayed
and not edited

Expected results:

the text in the editor window WYSIWYG HTML Editor should be edited

Trackers and Fingerprinters NOT enabled for me under about:preferences#privacy

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0
20190516215225

Please provide an example link that shows the problem. The following works for me (as long as Trackers and Fingerprinters are unchecked):
https://fmarier.github.io/brave-testing/tinymce.html

Also, can you reproduce the problem in a brand new profile?
https://support.mozilla.com/kb/profile-manager-create-and-remove-firefox-profiles

Status: RESOLVED → UNCONFIRMED
Component: Privacy: Anti-Tracking → User events and focus handling
Flags: needinfo?(adv)
Resolution: DUPLICATE → ---

(In reply to Gingerbread Man from comment #3)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0
20190516215225

Please provide an example link that shows the problem.

example http://www.imathas.com/editordemo/demo.html

working fine in 66.0.5 Version and before
not working in 67 Version

Flags: needinfo?(adv)

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2624b1bdee9b2bab41ca9a8c784cf0a7f14c2b7e&tochange=a01586b62cf510bb165057e0bea9a45cc76e961e

URL: http://www.imathas.com/editordemo/dem...
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Component: User events and focus handling → DOM: Core & HTML
Ever confirmed: true
Flags: needinfo?(bzbarsky)
Keywords: regression
Regressed by: 1489308

I will look into this on Tuesday (Monday is a holiday).

Hi,
we have the same problem in our old but still alive editor and have some problems migration the TinyMCE with all the plugins working.
So if there would be another way - that really would be great!

Thank you!

Same here. First noticed this in Developer Edition 67 and it still exists in 68.0b5. We're using TinyMCE 3.4.3.1. Tested in TinyMCE 4 and 5 and didn't see the same issue. We do not have a public sample, sorry, but behavior is same as demo pasted above.

OK, so here's what's going on here, on the demo page:

  1. The setupIframe function is called in the MCE code.
  2. This function takes different codepaths based on the value of a: in Chrome this value is false, but in Firefox it's true. That's because a is set to m.isGecko, so the codepath guarded by a is only taken in Firefox.
  3. The Firefox codepath does the following things, in order:
    a) Add an event listener for DOMContentLoaded to the iframe's current Window which will end up setting up all the content in the iframe via calling setupIframe again in a way that bypasses the Firefox-only codepath.
    b) Call open(), write(), close() on the iframe's document.
    c) Return.

The non-Firefox codepath is quite different. It doesn't mess around with DOMContentLoaded. Instead it just does the open(), write(), close() thing, then sets up the DOM in the iframe. Pretty straightforward. If I change the UA string in Firefox to not include the string "Gecko" in it, the demo starts working correctly, of course.

What used to happen before bug 1489308 was fixed, was the following:

  1. The DOMContentLoaded event listener got added on the Window in the iframe.
  2. The open() call reused that window, instead of (as usual) creating a new one, because the document in there was the initial about:blank. This is not at all guaranteed, in general; it just happens to work in this specific testcase, though it's possible that there's enough control over this iframe in MCE that it always works in practice.
  3. The close() call triggered a DOMContentLoaded event, the event listener fired, and things got set up.

After bug 1489308 was fixed, the open() call never creates a new Window, per spec. Also per spec, it clears all event listeners on the Window. So step (3b) above removes the listener that was added in step (3a), the listener is never called (though the event itself fires, of course), and the "set up the content in the iframe" bits are never reached.

I'm really not sure how to fix this while still following the spec, honestly, nor how the spec could be changed to make this demo work, given its Firefox-specific reliance on a fundamental thing browsers really do want to do on document.open (remove the event listeners from the Window involved) not happening. We could add some sort of "is the active script TinyMCE, if so don't remove DOMContentLoaded listeners during open()" thing, I guess, but that's about all I can think of at the moment.

It looks like more recent versions of TinyMCE have already fixed this code, as comment 8 notes. In particular, I tried grabbing some old versions from https://www.nuget.org/packages/TinyMCE and it looks like 3.4.4 has the DOMContentLoaded bits but 3.4.5 does not. It looks like that code was trying to work around some issue in very old Firefox releases that is long-since fixed; TinyMCE 3.4.5 was released in September 2011, so it sounds like already by that point whatever issue it was wasn't present in any Firefox versions people were still using.

In terms of site fixes, the simplest is updating TinyMCE; an update to 3.4.5 or later should fix things. If that's too much risk, the simplest possible change is to edit tiny_mce.js and move the z.open() call to above the if (a && !v.readonly) { block instead of coming right after it inside the setupIframe function. At least that's what the minifier is calling the variables on the demo site linked in comment 4. In terms of the original script this corresponds to moving d.open() to right before the if (isGecko && !s.readonly) { block. That would ensure that the event listener is added after the open() call and hence gets called.

Flags: needinfo?(bzbarsky)
Summary: TinyMCE doesn't work in Firefox 67 → TinyMCE versions 3.4.4 and older don't work in Firefox 67

I guess the other possible site fix would be to change the value of isGecko to false in tiny_mce.js. I don't know what other knock-on effects that would have, though, and whether anything conditioned on that is still needed in recent Firefox. The fix I describe in comment 10 is definitely much safer.

If some of the bug reporters here are customers of TinyMCE, would it be possible to see if they can release 3.4.3.x and 3.4.4.x versions with that fix from comment 10 but no other changes, so ideally people can update with them without the problems comment 7 mentions? Seems dubious, for 8-year-old versions, but....

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)

If some of the bug reporters here are customers of TinyMCE, would it be possible to see if they can release 3.4.3.x and 3.4.4.x versions with that fix from comment 10 but no other changes, so ideally people can update with them without the problems comment 7 mentions? Seems dubious, for 8-year-old versions, but....

Hi,

thanks for your analysis and comments we could solve the problem with both of your tiny_mce.js-patches and the TinyMCE version 3.5.12!
It works again now :-)

You made our day !

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11)

I guess the other possible site fix would be to change the value of isGecko to false in tiny_mce.js.

It works now. Thank you!

I think it would be better to make a fix or patch in the browser specifically for this bug. Not everyone can fix it. And not everyone will understand what stopped working because of the browser update. And version 3.4.4 was very popular and was added to various site engines.

I think it would be better to make a fix or patch in the browser specifically for this bug.

OK, but how, specifically, do you propose we fix it? That is the rub...

We should definitely add a release note for this. I kinda wish this had gotten reported before 67 shipped so we could have had it in the initial release notes there....

This isn't going into the Trailhead "67.0.5" release, but I'll leave it on the radar for possible 67 dot release inclusion if a low-risk patch becomes available. Boris, can you please suggest wording for the relnote you'd like added?

status-firefox67.0.1: affected → wontfix
tracking-firefox67: ? → +
tracking-firefox67.0.1: ? → -
tracking-firefox68: ? → +
tracking-firefox69: ? → +

Something like this:

TinyMCE versions 3.4.4 and earlier are not compatible with Firefox 67 and later, because they have a Firefox-specific codepath that assumes non-standard behavior and that behavior was removed. This can be addressed by updating to TinyMCE 3.4.5 or later, or by modifying the setupIframe() function in tiny_mce.js to have the d.open() call right before the big conditional block that adds a DOMContentLoader event listener, not right after that block.

Priority: -- → P2

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

Something like this:

TinyMCE versions 3.4.4 and earlier are not compatible with Firefox 67 and later, because they have a Firefox-specific codepath that assumes non-standard behavior and that behavior was removed. This can be addressed by updating to TinyMCE 3.4.5 or later, or by modifying the setupIframe() function in tiny_mce.js to have the d.open() call right before the big conditional block that adds a DOMContentLoader event listener, not right after that block.

Added to the 67.0 release notes as a known issue.

relnote-firefox: ? → 67+
Keywords: site-compat

Boris told me via IRC that he and Thomas Wisniewski are discussing an option via email and they'll report back here with a plan.

add a webcompat intervention for TinyMCE 3.0 thru 3.4.4 to overwrite tiny_mce.js and tiny_mce_src.js such that isGecko=false

I'm going to give DenSchub a chance to look over this patch before I land it, but barring any other feedback I may receive I think it's ready to land.

DenSchub just gave me a thumbs-up on IRC, so I'm going to queue the patch for landing.

Pushed by twisniewski@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c48327ec2874
add a webcompat intervention for TinyMCE 3.0 thru 3.4.4 to overwrite tiny_mce.js and tiny_mce_src.js such that isGecko=false; r=bzbarsky,Gijs

Backed out changeset c48327ec2874 (bug 1554014) for Browser-chrome failures in browser/components/newtab/test/browser/browser_asrouter_targeting.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251944142&repo=autoland&lineNumber=13274

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c48327ec28745ebfda5d660b5297ca69109f78fa

Backout:
https://hg.mozilla.org/integration/autoland/rev/daf5a6da71b6c7f9b5f493e0d6fccf2f73241f2f

Flags: needinfo?(twisniewski)

Hmm. I'm afraid that this one is beyond me. It doesn't reproduce for me locally with --verify, though I didn't test on a win7 box as I don't have one.

kmag, would you have any clues as to why this patch (which modifies a system addon to use a webRequest filter to potentially rewrite scripts named tiny_mce.js or tiny_mce_src.js) would cause this assert-trip/stack trace, and apparently only on win7? https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251944142&repo=autoland&lineNumber=13274

Flags: needinfo?(twisniewski) → needinfo?(kmaglione+bmo)

FWIW this sounds like that code is completely unsound, and your patch I think is just responsible for making that JS code slow enough such that it gets interrupted by the JS engine on automation.

PaintWhileInterruptingJS is interrupting some JS-implemented nsContentPolicy that runs during layout. That's not sound, you're not supposed to paint during layout. I think we have a bug on file for this already...

sigh

There's another bug about this that I can't find right now.

Essentially, the problem is that we open channels when loading images during reflow. Opening channels has the potential to run JS in a variety of ways. And that JS has the potential to do all sorts of things that aren't allowed during reflow.

In this case, that appears to just be allowing an interrupt callback to run and attempt to paint. Which is obviously not OK when we're in the middle of a layout flush.

Flags: needinfo?(kmaglione+bmo)

Any thoughts on this, bz? I worry whether this patch is possible to land in 68 now, if it slows things down enough to increase the intermittents we're already seeing on that bug.

Flags: needinfo?(bzbarsky)

Can we do explicit performance measurement on this patch? I'm surprised there's a noticeable impact at all, but maybe I don't understand the infrastructure we're using here well enough.

Flags: needinfo?(bzbarsky) → needinfo?(twisniewski)

We should ideally not interrupt script running from layout, or such, probably...

(In reply to Emilio Cobos Álvarez (:emilio) from comment #32)

We should ideally not interrupt script running from layout, or such, probably...

We really shouldn't run JS during layout at all.

Since we aren't realistically going to get this intervention working in time for 68, I'm dropping the 68 tracking flag.

tracking-firefox68: + → ---
Flags: needinfo?(twisniewski)
Assignee: nobody → twisniewski
status-firefox68: affected → wontfix
tracking-firefox68: --- → +

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:twisniewski, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(twisniewski)
Attachment #9071726 - Attachment is obsolete: true

The patch has been abandoned, so I'm clearing the ni?

Flags: needinfo?(twisniewski)
Assignee: twisniewski → nobody

Are we planning to do anything for 69? Or later?

Flags: needinfo?(twisniewski)

Not to my knowledge. Based on comments 28, 32 and 33 it sounds like there is an underlying problem that we need to resolve (or find a work-around for) before this patch can work without triggering test-failures, and I don't have to know-how to fix those underlying problems.

Flags: needinfo?(twisniewski)

Do we know of any real sites that are affected by this? Or only demo pages? We can try outreach for affected sites until we come up with a solution.

(In reply to Mike Taylor [:miketaylr] from comment #39)

Do we know of any real sites that are affected by this? Or only demo pages? We can try outreach for affected sites until we come up with a solution.

We were one of the real sites and I was one of the folks who reported the problem. Our solution, since it seemed like the fix might take a while to come or not come at all, was to update our TinyMCE solution. I could probably roll back a dev space version for testing if that were useful. Not sure about making it publicly accessible though...

Thanks bill -- appreciate it. I think you made the right decision by upgrading, and would like to encourage other affected sites to do the same. :)

No need to roll back anything, we can reproduce the issue using the older versions.

Julien, I don't think we're likely to get in a fix for 69 (or 70, perhaps), especially without a plan or confidence of impact. Should we untrack it?

(Or, Ryan, if he sees this once he's back from PTO)

status-firefox69: affected → wontfix
status-firefox70: --- → affected
Flags: needinfo?(jcristau)
status-firefox70: affected → fix-optional
tracking-firefox68: + → ---
tracking-firefox69: + → ---
Flags: needinfo?(jcristau)

We are currently impacted by this change, and don't have a quick way to update TinyMCE. The tool itself is behind authentication but it would be appreciated if this was addressed in a future release of Firefox.

john.miller, comment 10 has some suggestions for changes you can make to your local TinyMCE copy short of a complete update.

Just to be clear, a "fix" for this in Firefox would consist in us (1) identifying that a script being included in the page is one of the broken TinyMCE versions and (2) trying to rewrite it ourselves on the fly to fix it, in a programmatic way. This is much riskier and more likely to break your tool than making manual edits to TinyMCE would be...

Boris -

Thanks so much for the quick reply on this - it helps me drive roadmap changes in our product to know it is an unlikely change for the Firefox team to make.

Priority: P2 → P3

its now in the 67 ESR, while in 78 its working...

79.0 broke again! does not work!

Can you post a URL that doesn't work so that we can look into it? It's not clear that this is the same problem as described here.

79.0 broke again! does not work! Please (In reply to Emilio Cobos Álvarez (:emilio) from comment #48)

Can you post a URL that doesn't work so that we can look into it? It's not clear that this is the same problem as described here.

Exactly the same problem. All the same before.
example http://www.imathas.com/editordemo/demo.html
Button "HTML Source Editor" does not click or open

http://gamebomb.ru/files/galleries/001/8/89/356645.jpg

Flags: needinfo?(emilio)

Well, that test-case is the same as the one in comment 4, and it hasn't been fixed on our side because of comment 10.

So it's not really surprising? I just bisected it and this has never worked since 67, because that URL hasn't been updated as described in that last comment. If you rely on an old version of TinyMCE you need to fix up the script as described in comment 10.

Flags: needinfo?(emilio)

It still worked yesterday. And it worked throughout all versions. Doesn't work today. Apparently after a new update.

Flags: needinfo?(emilio)

Do you know which version of Firefox were you using before/after the update?

Flags: needinfo?(emilio) → needinfo?(adv)

Exactly, I remembered. We changed:
isGecko=false

And after that it worked. But after today's update it stopped working. Then it's something else.

We have not changed anything at home. Something has changed in the browser.

Yesterday it still worked. So today's update 79.0 has broken.

Flags: needinfo?(adv)

It's strange. I just tried changing back to isGecko = false and it worked!

Sorry!

It's strange. I just tried changing back to isGecko = true and it worked!

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: