Firefox hangs when setting innerHtml in rich text editor

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: miroslav.ruza, Assigned: masayuki)

Tracking

(Depends on 2 bugs, Blocks 1 bug, {regression})

33 Branch
mozilla55
x86_64
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [qf:p1]btpp-followup-2016-03-03)

Attachments

(6 attachments)

Reporter

Description

3 years ago
Posted file files.zip
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

Saving changes in a document with certain content in our application hangs Firefox.

You can reproduce the problem by following steps:
1) Download and install the evaluation version of Polarion ALM from https://www.polarion.com/
2) Open the application in the Firefox
3) Select some project in the left navigation panel (for example E-Library) 
4) Select "Documents & Pages" in the navigation panel
5) Click the [+] button in the toolbar
6) Click "Word document" under "Import" in the opened dialog
7) Select the test.docx file from attached zip file, click Next
8) Wait for preview to load and then click "Import >>" 
9) Wait till the document opens
10) Do some trivial change in the content of the document
11) Press save button (the first one in the toolbar)

Additional information:
- It can be reproduced using new Firefox profile
- It does happen only with certain content in the document, not always
- Reducing document size by deleting part of it causes that the Firefox hangs only sometimes
- I have tried it in older versions of Firefox and it seems that it started happening in Firofox 33
- I have used the profiler in the developer tools and the result profile.json is in the attached zip. It shows 31.8 seconds spent in Parsing HTML
- I have also profiled it using the Gecko Profiler extension in the Nightly and the result is https://cleopatra.io/#report=dd2d190b97b35e739241e87b0ba9c5b84d41cf39

Information from about:support

Application Basics
------------------
 
Name: Firefox
Version: 47.0a1
Build ID: 20160222030212
Update Channel: nightly
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
OS: Windows_NT 6.1 x86-64
Multiprocess Windows: 1/1 (Enabled by default)
Safe Mode: false
 
Crash Reports for the Last 3 Days
---------------------------------
 
All Crash Reports (including 7 pending crashes in the given time range)
 
Extensions
----------
 
Name: Firefox Hello Beta
Version: 1.1.2
Enabled: true
ID: loop@mozilla.org
 
Name: geckoprofiler
Version: 1.16.14
Enabled: true
ID: jid0-edalmuivkozlouyij0lpdx548bc@jetpack
 
Name: Pocket
Version: 1.0
Enabled: true
ID: firefox@getpocket.com
 
Graphics
--------
 
Adapter Description: Intel(R) HD Graphics 4000
Adapter Drivers: igdumd64 igd10umd64 igd10umd64 igdumd32 igd10umd32 igd10umd32
Adapter RAM: Unknown
Asynchronous Pan/Zoom: wheel input enabled; touch input enabled
Device ID: 0x0162
Direct2D Enabled: true
DirectWrite Enabled: true (6.2.9200.17568)
Driver Date: 3-19-2012
Driver Version: 8.15.10.2696
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 11 (OMTC)
Subsys ID: 21111462
Supports Hardware H264 Decoding: Yes
Vendor ID: 0x8086
WebGL Renderer: Google Inc. -- ANGLE (Intel(R) HD Graphics 4000 Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote: true
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0
 
Important Modified Preferences
------------------------------
 
browser.cache.disk.capacity: 358400
browser.cache.disk.filesystem_reported: 1
browser.cache.disk.smart_size.first_run: false
browser.cache.disk.smart_size.use_old_max: false
browser.cache.frecency_experiment: 3
browser.download.importedFromSqlite: true
browser.places.smartBookmarksVersion: 7
browser.sessionstore.upgradeBackup.latestBuildID: 20160222030212
browser.startup.homepage_override.buildID: 20160222030212
browser.startup.homepage_override.mstone: 47.0a1
dom.apps.reset-permissions: true
dom.mozApps.used: true
extensions.lastAppVersion: 47.0a1
gfx.crash-guard.d3d11layers.appVersion: 44.0.2
gfx.crash-guard.d3d11layers.deviceID: 0x0162
gfx.crash-guard.d3d11layers.driverVersion: 8.15.10.2696
gfx.crash-guard.d3d11layers.feature-d2d: true
gfx.crash-guard.d3d11layers.feature-d3d11: true
gfx.crash-guard.status.d3d11layers: 2
gfx.crash-guard.status.d3d9video: 2
gfx.direct3d.last_used_feature_level_idx: 0
media.gmp-eme-adobe.abi: x86_64-msvc-x64
media.gmp-eme-adobe.lastUpdate: 1456237184
media.gmp-eme-adobe.version: 16
media.gmp-gmpopenh264.abi: x86_64-msvc-x64
media.gmp-gmpopenh264.lastUpdate: 1456237184
media.gmp-gmpopenh264.version: 1.5.3
media.gmp-manager.buildID: 20160222030212
media.gmp-manager.lastCheck: 1456237183
media.hardware-video-decoding.failed: false
network.cookie.prefsMigrated: true
network.predictor.cleaned-up: true
places.history.expiration.transient_current_max_pages: 104858
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
privacy.sanitize.migrateClearSavedPwdsOnExit: true
security.sandbox.content.tempDirSuffix: {d6d39f7e-4f0f-42e6-8c2c-2fadd5e694bd}
storage.vacuum.last.index: 0
 
Important Locked Preferences
----------------------------
 
JavaScript
----------
 
Incremental GC: true
 
Accessibility
-------------
 
Activated: false
Prevent Accessibility: 0
 
Library Versions
----------------
 
NSPR
Expected minimum version: 4.12 Beta
Version in use: 4.12 Beta
 
NSS
Expected minimum version: 3.23 Basic ECC Beta
Version in use: 3.23 Basic ECC Beta
 
NSSSMIME
Expected minimum version: 3.23 Basic ECC Beta
Version in use: 3.23 Basic ECC Beta
 
NSSSSL
Expected minimum version: 3.23 Basic ECC Beta
Version in use: 3.23 Basic ECC Beta
 
NSSUTIL
Expected minimum version: 3.23 Beta
Version in use: 3.23 Beta
 
Experimental Features
---------------------


Actual results:

Firefox hangs and sometimes it recovers after a minute


Expected results:

Firefox does not hang and shows the refreshed document after few seconds.
Reporter

Updated

3 years ago
OS: Unspecified → Windows
Hardware: Unspecified → x86_64

Comment 1

3 years ago
Did it use to work in previous versions of Firefox?

Is it reproducible with a fresh profile?
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles
Flags: needinfo?(miroslav.ruza)
Reporter

Comment 2

3 years ago
Hi, Answers to both questions are already in my original post. So to repeat:

- I have tried it in several older versions and it does work in 29, 31, 32, but does not in 33, 34, 43, 44, 47

- yes it is reproducible using a fresh profile
Flags: needinfo?(miroslav.ruza)
Component: Untriaged → Editor
Product: Firefox → Core

Comment 3

3 years ago
As you have the system to reproduce it, could you use the tool Mozregression to find a regression range.
See http://mozilla.github.io/mozregression/ for details.
You need to install python 2.7 to run this package (use the command-line version if you want).

If FF32 is the last version which worked, run "mozregression --good=32" then copy here the final pushlog provided in the console output.
Flags: needinfo?(miroslav.ruza)

Comment 4

3 years ago
Does Polarion in the cloud has the same issue? (so I could test easily)
Reporter

Comment 5

3 years ago
Yes, Polarion in the cloud has the same issue, but I thought that it is easier to install the evaluation version. It is basically just few clicks installation on Windows.

I will try the Mozregression tomorrow and post the result.
Reporter

Comment 6

3 years ago
So, I have used the Mozregression and this is the result:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb35d1b73634&tochange=37f08ddaea48
Flags: needinfo?(miroslav.ruza)

Comment 7

3 years ago
So it regressed since FF33 and nobody reported since this date? Do you have a large base of customers using Polarion with Firefox? If not, don't expect to have a fix IMHO.

That said, I tried the web demo (because I don't want to spend time to install a 300MB software only for testing) and after importing the docx, the web app displayed an error message...
Whiteboard: btpp-followup-2016-03-03

Comment 8

3 years ago
(In reply to Loic from comment #7)
> So it regressed since FF33 and nobody reported since this date? Do you have
> a large base of customers using Polarion with Firefox? If not, don't expect
> to have a fix IMHO.
> 
> That said, I tried the web demo (because I don't want to spend time to
> install a 300MB software only for testing) and after importing the docx, the
> web app displayed an error message...

Surprisingly, noone reported so far from our customers, but it happened to me recently with two documents I obtained from our customers. Have to admit that I could not believe the regression is so old.

Polarion recommends Firefox as first choice for clients, so roughly less than a half of our user base will be using Firefox in their daily work. The issue may be, however, very data specific. From my product owner position, I'm willing to wait for further reports by our customers to see if it actually affects them. Usually, the customers who can use Firefox do not have a strict browser policy, so they can also use e.g. Chrome as a workaround for the issue. Still, I keep an eye on this and similar problems to determine if we should revisit our browser recommendation for future.

What demo server did you use and what is your user ID? We can prepare the data for you for easier reproduction of the problem. Just let us know.

Comment 9

3 years ago
I used this online demo of Polarion: http://reqdemo.polarion.com/polarion/
With this guest account:
U:bugzilla@yopmail.net
P:azerty123

I tried today again and the demo works fine, not like the other day.
So I used Mozregression and I can confirm your regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb35d1b73634&tochange=37f08ddaea48

After this range, saving the file freezes Firefox and the save process never ends.

Alice, any idea about the regressing bug?
Component: Editor → JavaScript Engine
Version: 47 Branch → 33 Branch

Comment 10

3 years ago
(In reply to Loic from comment #9)

> 
> Alice, any idea about the regressing bug?

No idea. Could you provide detailed STR?

Comment 11

3 years ago
(In reply to Alice0775 White from comment #10)
> (In reply to Loic from comment #9)
> 
> > 
> > Alice, any idea about the regressing bug?
> 
> No idea. Could you provide detailed STR?

0) Download the attached archive and unzip the test.docx file
1) Load http://reqdemo.polarion.com/polarion/
2) Log in with
U:bugzilla@yopmail.net 
P:azerty123
3) Use the down arrow of "Default Repository" to select the repository called "bugzilla_yopmail.net"
4) Click on the panel entry "Documents & Pages"
5) Click on the icon "+" at the top to upload a file
6) Under "Import", click on "Word Document"
7) Attach the test.docx file and click on "Next"
8) After the upload, click on "IMPORT >>" at the bottom (you'll see a "Progress" pop-up during 2-3 min)
9) Click inside the document and write random characters
10) Click on "Save" at the top

Result: the progress circle during saving the file works 15 sec then Firefox hangs and it's impossible to switch between tabs.

NB: you can delete the imported file "test.docx" by expanding "Default Space" under "Documents & Pages" and clicking on "Index", you'll see a list of files.

Updated

3 years ago
Component: JavaScript Engine → HTML: Parser

Comment 13

3 years ago
Suggested fix/workaround:

When setting content of TextArea
1. Detach TextArea from DOM
2. Set content of TextArea
3. Attach TextArea back to DOM

Do you think it could be fixed as suggested?
Marking confirmed per previous comments.

Assuming that this really is about textarea, as comment 13 suggests, smaug, do you see what in https://hg.mozilla.org/integration/mozilla-inbound/rev/48b9c15368aa could cause brokenness with textarea. On surface, it looks like, DoneAddingChildren() is called on textarea before and after that changeset.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bugs)

Comment 15

3 years ago
(In reply to Henri Sivonen (:hsivonen) from comment #14)
It is about iframe. (Our TextArea is on Firefox implemented using iframe.)
So it is about setting iframe.contentWindow.document.body.innerHTML = content
When iframe is detached/attached from/to DOM, setting iframe content is fast.
Trying to use the STR from comment 11, but there is no bugzilla_yopmail.net and I can't figure out how to add such or how to import anything.
Loic, Vladimir, could you perhaps help here.
Hard to say anything about the bug when I can't figure out how to reproduce it.


(based on the profile this is possibly all about spellchecking, but why... dunno yet.)
Flags: needinfo?(vladimir.hajek)
Flags: needinfo?(epinal99-bugzilla2)
Flags: needinfo?(bugs)

Comment 17

3 years ago
I will try to prepare simple javascript snippet which will reproduce the problem.
Flags: needinfo?(vladimir.hajek)

Comment 18

3 years ago
Let me share a few additional details from our customer that may be relevant for investigation.

Problem: Saving of some documents takes a great amount of time. It seems to be at least related to:
•	the document content
•	the browser

Firefox literally hangs up for a longer period of time, while the Polarion Server has finished the saving process long ago. Then, after waiting a long time, the hang-up situation recovers without any user interaction.
In fact, during this hang-up phase, it is not possible to use firefox, not even on other tabs, as changing tabs is not possible during the hang-up phase.
The problem can sometimes be avoided, if you switch to another tab in Firefox browser, directly after pressing the save button of the affected document, and waiting for some time, say 20 seconds. Then, when switching back to the previous tab, the hang-up has been avoided.
Sure, but I need some way to reproduce the issue before fixing it is possible.
My guess is, based on the cleopatra profile, that somehow spellchecker is triggered way too many times in a loop.

Comment 20

3 years ago
(In reply to Olli Pettay [:smaug] from comment #16)
> Trying to use the STR from comment 11, but there is no bugzilla_yopmail.net
> and I can't figure out how to add such or how to import anything.
> Loic, Vladimir, could you perhaps help here.
> Hard to say anything about the bug when I can't figure out how to reproduce
> it.
> 
> 
> (based on the profile this is possibly all about spellchecking, but why...
> dunno yet.)

New STR:

0) Download the attached archive and unzip the test.docx file
1) Load http://reqdemo.polarion.com/polarion/
2) Log in with
U:bugzilla@yopmail.net 
P:azerty123
3) Use the down arrow of "Default Repository" to select the repository called "bugzilla_yopmail.net2"
4) Click on the panel entry "Documents & Pages"
5) Click on the icon "+" at the top to upload a file
6) Under "Import", click on "Word Document"
7) Attach the test.docx file and click on "Next" (it can takes a few min for loadig the document)
8) After the upload, click on "IMPORT >>" at the bottom (you'll see a "Progress" pop-up during 2-3 min)
9) Click inside the document and write random characters
10) Click on "Save" at the top

NB: Steps 7/8 can be really long.
Flags: needinfo?(epinal99-bugzilla2)

Comment 21

3 years ago
(In reply to Olli Pettay [:smaug] from comment #16)

Problem is not caused by spellchecking - it occures even when spellchecking is off.

Unfortunatelly, I wasn't able prepare simple javascript to reproduce the problem.
It is not caused simple by setting iframe.contentWindow.document.body.innerHTML = content
There has to be something else e.g. iframe in some special state / specifically configured

Comment 22

3 years ago
Posted file semi-reduced html

Updated

3 years ago
Attachment #8787581 - Attachment description: index.html → semi-reduced html

Comment 23

2 years ago
Posted file FreezeFirefox.zip
Finally, I was able create simple javascript to reproduce the problem.

Problematic sequence which froze Firefox is:
iframe.contentWindow.document.designMode = 'On';
iframe.focus();
iframe.contentWindow.document.body.innerHTML = htmlContent;

Suspected commits which could cause the regression:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b6408c32a170&tochange=83c09fe3a658

Comment 24

2 years ago
(In reply to Vladimír Hájek from comment #23)

There is one more problematic sequence which froze Firefox:
Manually focus iframe by mouse click
iframe.focus();
iframe.contentWindow.document.body.innerHTML = htmlContent;

Comment 25

2 years ago
ni?smaug per comment 23.
Flags: needinfo?(bugs)
Ok, I wasn't sure what to do with the testcase, but looks like pressing 'focus & update' is enough.


Masayuki, all the time is spent under 
IMEContentObserver::NotifyContentAppended, and there mostly in ContentEventHandler::GetFlatTextLengthInRange
Flags: needinfo?(bugs) → needinfo?(masayuki)
oh, wait, no I got very different result, but I'm not getting hand either.

ok, looks like I need to first click the text area and then 'focus & update' button

Comment 28

2 years ago
(In reply to Olli Pettay [:smaug] from comment #27)

How to reproduce the problem using FreezeFirefox.html:

1. Check "Design Mode"
2. Press "Focus & Update"
OR
1. Mouse click to "TextArea"
2. Press "Focus & Update"

Also you can try repeat above scenarios and press "Update" instead of "Focus & Update".
Notice, that problem doesn't occur.

=> Problem occur only when iframe.focus() is called before setting iframe innerHtml.
Maybe IMEContentObserver should be nsIDoumentObserver and call
ContentEventHandler::GetFlatTextLengthInRange only when EndUpdate is being called.
Before that it would just somehow cache the data it has received in NotifyContentAdded or so.

Given that this is a major performance issue, P1.
Priority: -- → P1
(In reply to Olli Pettay [:smaug] from comment #26)
> Masayuki, all the time is spent under 
> IMEContentObserver::NotifyContentAppended, and there mostly in
> ContentEventHandler::GetFlatTextLengthInRange

Yes, could be. IMEContentObserver observes focused editor. So, inserting a big HTML fragment may cause performance issue (In other words, you can avoid this issue if the editor is blurred while inserting too big HTML fragment into it).

(In reply to Olli Pettay [:smaug] from comment #29)
> Maybe IMEContentObserver should be nsIDoumentObserver and call
> ContentEventHandler::GetFlatTextLengthInRange only when EndUpdate is being
> called.

If the range changed during nsIDocumentObserver::BeginUpdate() and nsIDocumentObserver::EndUpdate() is available, it's possible.  Otherwise, need to fix bug 1288613.

> Before that it would just somehow cache the data it has received in
> NotifyContentAdded or so.

Yes, ContentEventHandler needs to compute text length from start of the document because of difference between native line breaker's length and XP line breaker's length.  Therefore, if NotifyContentAdded() is called continuously at inserting multiple nodes once, IMEContentObserver caches the previous added node and its offset for reducing the redundant cost in ContentEventHandler.

However, according to the testcase (attachment 8848033 [details]), the cache doesn't work since inserting nodes have hierarchy, not only siblings in a container node.

If it's guaranteed that NotifyContentAdded() is called from first node to last node in the changed range, IMEContentObserver may be able to cache the range and compute only once at EndUpdate() (Perhaps, same behavior should be necessary at removing a range).
Flags: needinfo?(masayuki)
Hmm, looks like that if IMEContentObserver (or its helper class) becomes a document observer, it also receives same content changes too. Like the testcase, doesn't it cause another performance issue due to increased virtual calls?

IMEContentObserver needs to observer under an element. Therefore, if it's just a document observer, it needs to check if detected change is under editor's root. This also could cause another performance issue if the tree is too deep.

If nsIMutationObserver has "EndUpdate()", there is no problem, though.
I guess one could have two observers, one for mutations and one only for Begin/EndUpdate. And both registered only when needed.
(In reply to Olli Pettay [:smaug] from comment #32)
> I guess one could have two observers, one for mutations and one only for
> Begin/EndUpdate. And both registered only when needed.

Yes, possible but I worry about new overhead.

And there could be other performance issue at both pasting/deleting deep sub tree from HTML editor.

The removing case cannot be solved with nsIDocumentObserver because IMEContentObserver needs to know where *will* be removed before the nodes are actually removed from the tree.


Perhaps, another possible approach is, we can change FragmentOrElement::SetInnerHTMLInternal(), HTMLEditRules::WillInsertText(), DeleteRangeTransaction::DoTransaction() to notify IMEContentObserver of changed range. Then, IMEContentObserver can compute the range once. Perhaps, big performance issues may be caused only by them.
(In reply to Masayuki Nakano [:masayuki] from comment #33)
> The removing case cannot be solved with nsIDocumentObserver because
> IMEContentObserver needs to know where *will* be removed before the nodes
> are actually removed from the tree.
> 
That is why you have the mutation observer, no? Or do I misunderstand something here.


> Perhaps, another possible approach is, we can change
> FragmentOrElement::SetInnerHTMLInternal(), HTMLEditRules::WillInsertText(),
> DeleteRangeTransaction::DoTransaction() to notify IMEContentObserver of
> changed range. Then, IMEContentObserver can compute the range once. Perhaps,
> big performance issues may be caused only by them.
I doubt it is only about SetInnerHTML, but also about InsertAdjacentHTML etc., so some generic approach would be perhaps better.
(In reply to Olli Pettay [:smaug] from comment #34)
> (In reply to Masayuki Nakano [:masayuki] from comment #33)
> > The removing case cannot be solved with nsIDocumentObserver because
> > IMEContentObserver needs to know where *will* be removed before the nodes
> > are actually removed from the tree.
> > 
> That is why you have the mutation observer, no? Or do I misunderstand
> something here.

Currently, ContentRemoved() is optimized only for removing a lot of text nodes and <br> in same element.  Therefore, if deep subtree is being removed, ContentRemoved() will need to compute its start offset and its length at every call.  So, this cause same performance issue as this bug.

> > Perhaps, another possible approach is, we can change
> > FragmentOrElement::SetInnerHTMLInternal(), HTMLEditRules::WillInsertText(),
> > DeleteRangeTransaction::DoTransaction() to notify IMEContentObserver of
> > changed range. Then, IMEContentObserver can compute the range once. Perhaps,
> > big performance issues may be caused only by them.
> I doubt it is only about SetInnerHTML, but also about InsertAdjacentHTML
> etc., so some generic approach would be perhaps better.

Of course, if it's possible, it's the best.
And if we use nsIDocumentObserver, is it guaranteed that if ContentAppended() or ContentInserted() and ContentRemoved() are not called in a session? And also is it guaranteed that if these are called from former node to latter node and from ancestor node to descendant node? (Or do I misunderstand about removing container node case? ContentRemoved() won't be called with descendants of removed node (i.e., when <p>text</p> is removed, ContentRemoved() is called only once for <p>, not called for text node)?
Not sure what you mean with session, but Being/EndUpdate just tell that some change has been done. There can be several nsIMutationObserver calls between those, like in case of innerHTML:
http://searchfox.org/mozilla-central/source/dom/base/FragmentOrElement.cpp#2289,2295,2333

But I'm open to any approaches here :) (Just hoping it isn't hard to understand and maintain)

Updated

2 years ago
Blocks: 1346723
Whiteboard: btpp-followup-2016-03-03 → [qf]btpp-followup-2016-03-03

Updated

2 years ago
Whiteboard: [qf]btpp-followup-2016-03-03 → [qf:p1]btpp-followup-2016-03-03
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Summary: Firefox hangs when setting innertHtml in rich text editor → Firefox hangs when setting innerHtml in rich text editor
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Although, even with the patches, it's still slow. It almost spends in nsRange::ContentAppended():
https://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/dom/base/nsRange.cpp#601,609,611-612,617,620,629

And when I see the instance with debugger, it's not a selection range. Therefore, I have no idea how to optimize it. So, I'd like to put it off to other bug.
The stack when nsRnage::ContentAppended() is called:

>>	xul.dll!nsRange::ContentAppended(nsIDocument * aDocument, nsIContent * aContainer, nsIContent * aFirstNewContent, int aNewIndexInContainer) Line 609	C++
>  	xul.dll!nsNodeUtils::ContentAppended(nsIContent * aContainer, nsIContent * aFirstNewContent, int aNewIndexInContainer) Line 167	C++
>  	xul.dll!nsHtml5TreeOperation::Append(nsIContent * aNode, nsIContent * aParent, nsHtml5DocumentBuilder * aBuilder) Line 186	C++
>  	xul.dll!nsHtml5TreeOperation::AppendText(const char16_t * aBuffer, unsigned int aLength, nsIContent * aParent, nsHtml5DocumentBuilder * aBuilder) Line 167	C++
>  	xul.dll!nsHtml5TreeBuilder::appendCharacters(void * aParent, char16_t * aBuffer, int aStart, int aLength) Line 567	C++
>  	xul.dll!nsHtml5TreeBuilder::flushCharacters() Line 4504	C++
>  	xul.dll!nsHtml5TreeBuilder::endTag(nsHtml5ElementName * elementName) Line 2308	C++
>  	xul.dll!nsHtml5Tokenizer::emitCurrentTagToken(bool selfClosing, int pos) Line 327	C++
>  	xul.dll!nsHtml5Tokenizer::stateLoop<nsHtml5SilentPolicy>(int state, char16_t c, int pos, char16_t * buf, bool reconsume, int returnState, int endPos) Line 988	C++
>  	xul.dll!nsHtml5Tokenizer::tokenizeBuffer(nsHtml5UTF16Buffer * buffer) Line 445	C++
>  	xul.dll!nsHtml5StringParser::Tokenize(const nsAString & aSourceBuffer, nsIDocument * aDocument, bool aScriptingEnabledForNoscriptParsing) Line 115	C++
>  	xul.dll!nsHtml5StringParser::ParseFragment(const nsAString & aSourceBuffer, nsIContent * aTargetNode, nsIAtom * aContextLocalName, int aContextNamespace, bool aQuirks, bool aPreventScriptExecution) Line 62	C++
>  	xul.dll!nsContentUtils::ParseFragmentHTML(const nsAString & aSourceBuffer, nsIContent * aTargetNode, nsIAtom * aContextLocalName, int aContextNamespace, bool aQuirks, bool aPreventScriptExecution) Line 4984	C++
>  	xul.dll!mozilla::dom::FragmentOrElement::SetInnerHTMLInternal(const nsAString & aInnerHTML, mozilla::ErrorResult & aError) Line 2361	C++

Comment 43

2 years ago
mozreview-review
Comment on attachment 8874775 [details]
Bug 1250823 part 1 - Implement DocumentObserver as a nested class of IMEContentObserver for observing to begin and end update

https://reviewboard.mozilla.org/r/146150/#review150364

I think I'd like to see mIMEContentObserver usage clarified/fixed.

::: dom/events/IMEContentObserver.h:288
(Diff revision 1)
>  
>    // mQueuedSender is, it was put into the event queue but not run yet.
>    RefPtr<IMENotificationSender> mQueuedSender;
>  
>    /**
> +   * IMEContentObserver is a mutation observer of mRootContent.  However,

oh, so we just want Begin/EndUpdate calls, and 
IMEContentObserver wants only the DOM changes underneath it?

::: dom/events/IMEContentObserver.h:322
(Diff revision 1)
> +
> +  private:
> +    DocumentObserver() = delete;
> +    virtual ~DocumentObserver() { Destroy(); }
> +
> +    IMEContentObserver* mIMEContentObserver;

Why this isn't a strong reference which would be cycle collected?
I think it should be unless there is some good reason to not to.

::: dom/events/IMEContentObserver.cpp:390
(Diff revision 1)
>    }
>  
>    mEditor = nullptr;
>    mEditableNode = aContent;
>    mRootContent = aContent;
> +  mDocumentObserver = nullptr;

If 'this' dies after this and something keeps ref to mDocumentObserver, it has pointer to a deleted object in mIMEContentObserver.


In general, mIMEContentObserver needs a comment explaining why it is guaranteed to never point to a deleted object.
Attachment #8874775 - Flags: review?(bugs) → review-

Comment 44

2 years ago
mozreview-review
Comment on attachment 8874776 [details]
Bug 1250823 part 2 - IMEContentObserver should cache adding ranges as a range during document change

https://reviewboard.mozilla.org/r/146152/#review150366

This needs some clarifications, although I think I understand what this does.

::: dom/events/IMEContentObserver.h:402
(Diff revision 1)
> +  // mFirstAddedNodeContainer is not nullptr if a node was added during a
> +  // document change and has not been included into mTextChangeData yet.
> +  nsCOMPtr<nsINode> mFirstAddedNodeContainer;
> +  // mLastAddedNodeContainer is not nullptr if a node was added during a
> +  // document change and has not been included into mTextChangeData yet.
> +  nsCOMPtr<nsINode> mLastAddedNodeContainer;

These two nsCOMPtrs are guaranteed to have non-null values only temporarily, right?
So they don't need to be added to cycle collection.

::: dom/events/IMEContentObserver.h:402
(Diff revision 1)
> +  // mFirstAddedNodeContainer is not nullptr if a node was added during a
> +  // document change and has not been included into mTextChangeData yet.
> +  nsCOMPtr<nsINode> mFirstAddedNodeContainer;
> +  // mLastAddedNodeContainer is not nullptr if a node was added during a
> +  // document change and has not been included into mTextChangeData yet.
> +  nsCOMPtr<nsINode> mLastAddedNodeContainer;

I don't understand the difference between mFirstAddedNodeContainer and mLastAddedNodeContainer, at least not by reading the comments.

::: dom/events/IMEContentObserver.cpp:951
(Diff revision 1)
>      return;
>    }
>  
>    mEndOfAddedTextCache.Clear();
>    mStartOfRemovingTextRangeCache.Clear();
> +  MaybeNotifyIMEOfAddedTextDuringDocumentChange();

Why we need to call MaybeNotifyIMEOfAddedTextDuringDocumentChange both in CharacterDataWillChange and CharacterDataChanged?
This code needs some comments

::: dom/events/IMEContentObserver.cpp:999
(Diff revision 1)
>    mStartOfRemovingTextRangeCache.Clear();
>  
> +  // If it's in a document change, nodes are added consecutively.  Therefore,
> +  // if we cache the first node and the last node, we need to compute the
> +  // range once.
> +  if (IsInDocumentChange()) {

Is there ever a case when IsInDocumentChange() is false here?

::: dom/events/IMEContentObserver.cpp:1007
(Diff revision 1)
> +      mFirstAddedNodeContainer = mLastAddedNodeContainer = aContainer;
> +      mFirstAddedNodeOffset = aStartIndex;
> +      mLastAddedNodeOffset = aEndIndex;
> +      return;
> +    }
> +    nsIContent* startContent = aContainer->GetChildAt(aStartIndex);

Any chance to not use indexes here. We're trying to reduce their usage since they are slow.
(See bug 651120)

::: dom/events/IMEContentObserver.cpp:1209
(Diff revision 1)
>                        IsEditorComposing());
>    MaybeNotifyIMEOfTextChange(data);
>  }
>  
>  void
> +IMEContentObserver::MaybeNotifyIMEOfAddedTextDuringDocumentChange()

this method needs some comments. It isn't clear to me what this is trying to do.
Attachment #8874776 - Flags: review?(bugs) → review-
Comment on attachment 8874775 [details]
Bug 1250823 part 1 - Implement DocumentObserver as a nested class of IMEContentObserver for observing to begin and end update

https://reviewboard.mozilla.org/r/146150/#review150364

> oh, so we just want Begin/EndUpdate calls, and 
> IMEContentObserver wants only the DOM changes underneath it?

Yes. Any changes in focused editor is already listened by IMEContentObserver itself (it's a subclass of nsStubMutationObserver). Be careful, we do not need any DOM tree changes outside of focused editor. Therefore, we need only Begin/EndUpdate.

> Why this isn't a strong reference which would be cycle collected?
> I think it should be unless there is some good reason to not to.

Ah, good point. This is just my mistake.

> If 'this' dies after this and something keeps ref to mDocumentObserver, it has pointer to a deleted object in mIMEContentObserver.
> 
> 
> In general, mIMEContentObserver needs a comment explaining why it is guaranteed to never point to a deleted object.

Okay, I'll add comments and make the callers guarantee the lifetime of the instance.
Comment on attachment 8874776 [details]
Bug 1250823 part 2 - IMEContentObserver should cache adding ranges as a range during document change

https://reviewboard.mozilla.org/r/146152/#review150366

> These two nsCOMPtrs are guaranteed to have non-null values only temporarily, right?
> So they don't need to be added to cycle collection.

Oh, I forgot to make them into CC. On the other hand, as you said, they shouldn't be released and they stored only during a document change (even at most). So, I'll just add comments.

> Why we need to call MaybeNotifyIMEOfAddedTextDuringDocumentChange both in CharacterDataWillChange and CharacterDataChanged?
> This code needs some comments

I added following comment:

  // Although we don't assume this change occurs while this is storing
  // the range of added consecutive nodes, if it actually happens, we need to
  // flush them since this change may occur before or in the range.  So, it's
  // safe to flush pending computation of mTextChangeData before handling this.
  MaybeNotifyIMEOfAddedTextDuringDocumentChange();

and remove the call from IMEContentObserver::CharacterDataChanged() since it's too late to compute the range.

> Is there ever a case when IsInDocumentChange() is false here?

Yes. If the change occurs in editor (either platintext editor or contenteditable), this is false. I added it to the comment. Note that in such case, i.e., usual text input by edit actions were optimized with mEndOfAddedTextCache.

> Any chance to not use indexes here. We're trying to reduce their usage since they are slow.
> (See bug 651120)

Okay. But sounds like the children should be in independent array for faster access or nsINode should use GetFirstChild() or GetLastChild() if the index is obviously indicates one of them.

I put warnings if it hits slow path which uses GetChildAt() for the last resort, but I don't see them with innerHTML.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 52

2 years ago
mozreview-review
Comment on attachment 8874775 [details]
Bug 1250823 part 1 - Implement DocumentObserver as a nested class of IMEContentObserver for observing to begin and end update

https://reviewboard.mozilla.org/r/146150/#review150802

comments addressed, r+

::: dom/events/IMEContentObserver.h:99
(Diff revisions 1 - 2)
> +   */
>    void Init(nsIWidget* aWidget, nsPresContext* aPresContext,
>              nsIContent* aContent, nsIEditor* aEditor);
> +
> +  /**
> +   * Destroy() finalizes the instance, i.e., stopping observing contents and

s/stopping/stops/

::: dom/events/IMEContentObserver.h:107
(Diff revisions 1 - 2)
> +   * won't be released during calling this.
> +   */
>    void Destroy();
> +
> +  /**
> +   * Returns true if the instance refers some objects and observing them.

This is backwards. Certainly this should return false when we still observe something

::: dom/events/IMEContentObserver.h:357
(Diff revisions 1 - 2)
>  
>    private:
>      DocumentObserver() = delete;
>      virtual ~DocumentObserver() { Destroy(); }
>  
> -    IMEContentObserver* mIMEContentObserver;
> +    RefPtr<IMEContentObserver> mIMEContentObserver;

Don't you want to add mIMEContentObserver to cycle collection now.

::: dom/events/IMEContentObserver.cpp:207
(Diff revisions 1 - 2)
>    if (!firstInitialization) {
>      // If this is now trying to initialize with new contents, all observers
>      // should be registered again for simpler implementation.
>      UnregisterObservers();
>      // Clear members which may not be initialized again.
> -    Clear();
> +    RefPtr<IMEContentObserver> kungFuDeathGrip = this;

this is weird. When kungFuDeathGrip goes out of scope, we still access mESM which might be now a member variable of a deleted object.

Per documentation the caller of Init should keep the object alive, so why we need kungFuDeathGrip at all?

::: dom/events/IMEContentObserver.cpp:394
(Diff revisions 1 - 2)
>    mEditableNode = aContent;
>    mRootContent = aContent;
> +  // Should be safe to clear mDocumentObserver here even though it *might*
> +  // grab this instance because this is called by Init() and the callers of
> +  // it and MaybeReinitialize() grabs this instance with local RefPtr.
> +  // So, this won't cause refcount of this instance becoming 0.

s/becoming/become/
Attachment #8874775 - Flags: review?(bugs) → review+

Comment 53

2 years ago
mozreview-review
Comment on attachment 8874776 [details]
Bug 1250823 part 2 - IMEContentObserver should cache adding ranges as a range during document change

https://reviewboard.mozilla.org/r/146152/#review150806

::: dom/events/IMEContentObserver.h:186
(Diff revision 2)
>    bool IsReflowLocked() const;
>    bool IsSafeToNotifyIME() const;
>    bool IsEditorComposing() const;
>  
> +  /**
> +   * nsINode::GetChildAt() is slow.  So, this avoids to use it if it's

Technically GetChildAt is fast atm, but will be slower once we remove the child array.

::: dom/events/IMEContentObserver.h:472
(Diff revision 2)
> +  // mFirstAddedNodeContainer is parent node of first added node in current
> +  // document change.  So, this is not nullptr only when a node was added
> +  // during a document change and the change has not been included into
> +  // mTextChangeData yet.
> +  // Note that this shouldn't be in cycle collection since this is not nullptr
> +  // only in a document change.  So, it's enough short and shouldn't be cleared.

only during a document change.

And then you can drop
"So, it's enough short and shouldn't be cleared."

::: dom/events/IMEContentObserver.h:479
(Diff revision 2)
> +  // mLastAddedNodeContainer is parent node of last added node in current
> +  // document change.  So, this is not nullptr only when a node was added
> +  // during a document change and the change has not been included into
> +  // mTextChangeData yet.
> +  // Note that this shouldn't be in cycle collection since this is not nullptr
> +  // only in a document change.  So, it's enough short and shouldn't be cleared.

Same here
Attachment #8874776 - Flags: review?(bugs) → review+
Comment on attachment 8874775 [details]
Bug 1250823 part 1 - Implement DocumentObserver as a nested class of IMEContentObserver for observing to begin and end update

https://reviewboard.mozilla.org/r/146150/#review150802

> This is backwards. Certainly this should return false when we still observe something

Oops! Thanks!

> Don't you want to add mIMEContentObserver to cycle collection now.

Sure.

> this is weird. When kungFuDeathGrip goes out of scope, we still access mESM which might be now a member variable of a deleted object.
> 
> Per documentation the caller of Init should keep the object alive, so why we need kungFuDeathGrip at all?

Oh, it was added before I checked all callers. So, it is not necessary anymore as you said. I'll remove it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 57

2 years ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d786da7fddda
part 1 - Implement DocumentObserver as a nested class of IMEContentObserver for observing to begin and end update r=smaug
https://hg.mozilla.org/integration/autoland/rev/c02a8965ab7c
part 2 - IMEContentObserver should cache adding ranges as a range during document change r=smaug

Comment 58

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d786da7fddda
https://hg.mozilla.org/mozilla-central/rev/c02a8965ab7c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1396976

Updated

8 months ago
Depends on: 1500852
You need to log in before you can comment on or make changes to this bug.