TinyMCE versions 3.4.4 and older don't work in Firefox 67
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
People
(Reporter: adv, Unassigned)
References
(Regression, )
Details
(Keywords: regression, site-compat)
Attachments
(1 file, 1 obsolete file)
197.96 KB,
image/jpeg
|
Details |
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
Comment hidden (obsolete) |
Trackers and Fingerprinters NOT enabled for me under about:preferences#privacy
Comment 3•5 years ago
|
||
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
(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
20190516215225Please 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
Comment 5•5 years ago
|
||
regression-window |
Comment 6•5 years ago
|
||
I will look into this on Tuesday (Monday is a holiday).
Comment 7•5 years ago
|
||
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.
Comment 9•5 years ago
|
||
OK, so here's what's going on here, on the demo page:
- The
setupIframe
function is called in the MCE code. - 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 becausea
is set tom.isGecko
, so the codepath guarded bya
is only taken in Firefox. - The Firefox codepath does the following things, in order:
a) Add an event listener forDOMContentLoaded
to the iframe's currentWindow
which will end up setting up all the content in the iframe via callingsetupIframe
again in a way that bypasses the Firefox-only codepath.
b) Callopen()
,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:
- The
DOMContentLoaded
event listener got added on theWindow
in the iframe. - The
open()
call reused that window, instead of (as usual) creating a new one, because the document in there was the initialabout: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. - The
close()
call triggered aDOMContentLoaded
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.
Comment 10•5 years ago
•
|
||
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.
Updated•5 years ago
|
Comment 11•5 years ago
|
||
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.
Comment 12•5 years ago
|
||
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....
Comment 13•5 years ago
|
||
(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 !
Reporter | ||
Comment 14•5 years ago
|
||
(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
tofalse
intiny_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.
Comment 15•5 years ago
|
||
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....
Updated•5 years ago
|
Updated•5 years ago
|
Comment 16•5 years ago
|
||
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?
Comment 17•5 years ago
|
||
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 intiny_mce.js
to have thed.open()
call right before the big conditional block that adds aDOMContentLoader
event listener, not right after that block.
Updated•5 years ago
|
Comment 18•5 years ago
|
||
(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 intiny_mce.js
to have thed.open()
call right before the big conditional block that adds aDOMContentLoader
event listener, not right after that block.
Added to the 67.0 release notes as a known issue.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 19•5 years ago
|
||
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.
Comment 20•5 years ago
|
||
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
Comment 21•5 years ago
|
||
Comment 22•5 years ago
|
||
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.
Comment 23•5 years ago
|
||
DenSchub just gave me a thumbs-up on IRC, so I'm going to queue the patch for landing.
Comment 24•5 years ago
|
||
Comment 25•5 years ago
|
||
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
Comment 26•5 years ago
|
||
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
Comment 27•5 years ago
|
||
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...
Comment 28•5 years ago
|
||
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.
Comment 29•5 years ago
|
||
This is the bug I was thinking of:
Comment 30•5 years ago
|
||
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.
Comment 31•5 years ago
|
||
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.
Comment 32•5 years ago
|
||
We should ideally not interrupt script running from layout, or such, probably...
Comment 33•5 years ago
|
||
(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.
Comment 34•5 years ago
|
||
Since we aren't realistically going to get this intervention working in time for 68, I'm dropping the 68 tracking flag.
Updated•5 years ago
|
Comment 35•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 36•5 years ago
|
||
The patch has been abandoned, so I'm clearing the ni?
Updated•5 years ago
|
Comment 37•5 years ago
|
||
Are we planning to do anything for 69? Or later?
Comment 38•5 years ago
|
||
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.
Comment 39•5 years ago
|
||
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.
Comment 40•5 years ago
|
||
(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...
Comment 41•5 years ago
|
||
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.
Comment 42•5 years ago
|
||
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)
Updated•5 years ago
|
Comment 43•5 years ago
|
||
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.
Comment 44•5 years ago
|
||
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...
Comment 45•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 46•4 years ago
|
||
its now in the 67 ESR, while in 78 its working...
Reporter | ||
Comment 47•4 years ago
|
||
79.0 broke again! does not work!
Comment 48•4 years ago
|
||
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.
Reporter | ||
Comment 49•4 years ago
|
||
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
Reporter | ||
Comment 50•4 years ago
|
||
Comment 51•4 years ago
|
||
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.
Reporter | ||
Comment 52•4 years ago
|
||
It still worked yesterday. And it worked throughout all versions. Doesn't work today. Apparently after a new update.
Comment 53•4 years ago
|
||
Do you know which version of Firefox were you using before/after the update?
Reporter | ||
Comment 54•4 years ago
|
||
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.
Reporter | ||
Comment 55•4 years ago
|
||
Yesterday it still worked. So today's update 79.0 has broken.
Reporter | ||
Comment 56•4 years ago
|
||
It's strange. I just tried changing back to isGecko = false and it worked!
Reporter | ||
Comment 57•4 years ago
|
||
Sorry!
It's strange. I just tried changing back to isGecko = true and it worked!
Updated•2 years ago
|
Updated•2 years ago
|
Description
•