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.

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

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

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.

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

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.

So, I have used the Mozregression and this is the result: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb35d1b73634&tochange=37f08ddaea48

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...

(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.

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?

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

(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.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8222bfcd04fb&tochange=3a779ba12dad Suspect: 48b9c15368aa Olli Pettay — Bug 902618 - Parser notifies about subtree roots, but not their descendants, r=hsivonen,bz Stack(using crashfirefox.exe): bp-6199bfa7-6309-4ab5-8910-a37092160301 bp-ea4609c8-6b30-4a67-ac6e-9834b2160301 bp-cf4c32ed-1a32-422b-8b97-f2fb72160301 bp-18758b60-1974-4fcb-84ea-431de2160301 And also attached windbg log

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.

(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.)

I will try to prepare simple javascript snippet which will reproduce the problem.

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.

(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.

(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

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

(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;

ni?smaug per comment 23.

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

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

(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.

(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).

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)

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 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.

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.

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 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/

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

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.

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

https://hg.mozilla.org/mozilla-central/rev/d786da7fddda https://hg.mozilla.org/mozilla-central/rev/c02a8965ab7c