Status: UNCONFIRMED → NEW
tracking-firefox21: --- → ?
Component: General → DOM: Core & HTML
Ever confirmed: true
Product: Firefox → Core
My fix in bug 597525 revealed a few weakness in our HTMLTextAreaElement implementation. Basically, before, we were always reading the value from the content of the textarea anytime we were needing the value which was a waste of CPU given that we should theoretically have that value on hand all the time. That particular bug is about .cloneNode() that doesn't notify about any change so the textarea isn't aware of its content. The two solutions I see to fix this bug are: - Call ::DoneAddingChildren() more often (like when we clone a node); - Make nsHTMLTextAreElement call ::Reset() when InsertChildAt() is called. I think ::DoneAddingChildren() is a better solution but I wonder if that sounds correct for you guys (CC'd). Also, should call the method only for textarea or for other elements? I'm not sure how other elements with similar behaviour could be impacted by such bugs. They might have their own workarounds. Thank you for this bug report Yamada San.
Assignee: nobody → mounir
OS: Windows XP → All
Hardware: x86 → All
Given the next merge to Aurora is very soon, should we back out bug 597525 now, and reland later next week (with a fix for this bug)?
The point of the DoneAddingChildren was, iirc, so that we know when the parser is done creating the subtree under the <textarea>. This is needed because the parser doesn't fire nsIMutationObserver notifications. So I don't think we need that here. We should be receiving nsIMutationObserver notifications for all child insertions during cloning. And for each of those we should be updating the textarea as needed.
(In reply to Jonas Sicking (:sicking) from comment #4) > The point of the DoneAddingChildren was, iirc, so that we know when the > parser is done creating the subtree under the <textarea>. This is needed > because the parser doesn't fire nsIMutationObserver notifications. > > So I don't think we need that here. We should be receiving > nsIMutationObserver notifications for all child insertions during cloning. > And for each of those we should be updating the textarea as needed. Indeed, that's a third solution. I waved out because I had an issue with XBL and notifying was simply crashing Gecko. I should try that option.
(In reply to Olli Pettay [:smaug] from comment #3) > Given the next merge to Aurora is very soon, should we back out bug 597525 > now, and reland later next > week (with a fix for this bug)? Given that cloneNode() on Webkit doesn't seem to be doing a deep cloning, I do not think that this bug is going to be very bad and I'm pretty sure we will have a fix landed very soon so it could likely be uplifted to Aurora.
(In reply to Mounir Lamouri (:mounir) from comment #5) > Indeed, that's a third solution. I waved out because I had an issue with XBL > and notifying was simply crashing Gecko. I should try that option. It seems to work fine, sent to try. I will ask for a review if try passes cleanly. https://tbpl.mozilla.org/?tree=Try&rev=02276197a734
Comment on attachment 714949 [details] [diff] [review] Patch Ok, but please test printing/print preview performance. In case of printing HTML5 spec (one page version) this ends up doing *lots* of mutation observer calls, and those aren't necessarily very cheap (though, not as expensive as they used to be). hmm, in fact, please test cloning performance of some large-ish DOM subtrees. Pathological case should be a deep DOM tree where most of the nodes are leaf descendants of the root. Element to clone | Element | <deep Element hierachy> | Element / / | \ \ Node Node Node Node <lots of nodes ...> r+ *iff* there are no significant performance regressions. (but I can't quite see how this wouldn't cause at least some performance regressions.)
Attachment #714949 - Flags: review?(bugs) → review+
Created attachment 715142 [details] Performance testcase So, I wrote this performance test case. Without my patch, the first test has results between 35 to 40ms for input being '6'. The second test has a result of 6ms for 10'000 to 12'000 and 10ms for 15'000. With my patch, the first test has results between 45 to 50ms for the same input. The second test has a result of 8-10ms for 10'000 to 12'000 and 12ms for 15'000. There is clear regression here... Do we want to take this regression or do we want to use DoneAddingChildren() for a whitelist of elements? BTW, as a comparaison, Opera has results between 14 to 18ms for the first test case. It can't generate the tree quickly enough for the second test case so hard to compare here...
Status: NEW → ASSIGNED
Could we just back out bug 597525?
(In reply to Olli Pettay [:smaug] from comment #11) > Could we just back out bug 597525? Bug 597525 is removing some code that was needed only because of bugs like that. Basically, we were not able to have an up-to-date value for <textarea> so we were simply asking the textarea to re-parse its value when we need it (in the related code path). Given that the current philosophy of the code is that the textarea knows its value at any given time, re-parsing the value was simply a bug. I think we should whether stop assuming that the textarea knows its default at any time when it's value is the default value or fix bugs like this. I would be fine to backout bug 597525 if we go with the former solution. In that case, the change wouldn't be simple (because nsTextEditorState hasn't been designed that way). We we win some performance in some cases (no mutation observers) and lose some in others (.value would take more time if .value == .defaultValue). Anyway, Olli would like Jonas' opinion on this.
We need some improvement of the cloneNode code here. Surely textarea's aren't the only elements that rely nsIMutationObserver notifications.
But whatever improvement we do, it must happen in m-c, not in Aurora. So could we backout bug 597525 for now?
:mounir, can you please help prepare the back out , seems like the plan going forward per comment #14 .Correct ?
Spoke offline with :Mounir on this and he is going to backout bug 597525, hence no need to track this
tracking-firefox21: ? → -
status-firefox20: --- → unaffected
status-firefox21: --- → affected
status-firefox22: --- → affected
Version: Trunk → 21 Branch
Bug 597525 has been backed out on aurora and central.
OK, confirmed fixed by backout of bug 597525. Thank you for your great works.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox21: affected → fixed
status-firefox22: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0 Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130426 Firefox/23.0 Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:23.0) Gecko/20130426 Firefox/23.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130426 Firefox/23.0 Verified as fixed on Firefox 21 (Build ID:20130425162858) and on Firefox latest nightly (Build ID:20130426030834).
You need to log in before you can comment on or make changes to this bug.