841882 - No text in textarea of edit page on some wiki sites using pukiwiki

Status: RESOLVED FIXED

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

Description

[Toshihiro Yamada]

On some wiki sites using pukiwiki(one of famous wiki software in Japan),
<textarea> in edit page doesn't show source of the page at all.
This problem happens on wiki sites using pukiwiki and Javascript to construct editor with click-able button to help writing source as far as I tested. 

Step to reproduce
1. Open http://popn.sakura.ne.jp/m/index.php?Lv41%2F%E5%A4%A9%E7%A9%BA%E3%83%AF%E3%83%AB%E3%83%84%28EX%29
2. Click "編集" in the header of the page to open edit page with Javascript enabled.

Actual Result
<textarea> in the edit page doesn't show any text at all.
(Although <textarea> has text node correctly according to inspector)

Expected result
<textarea> shows the source of the page correctly.

Regression range(m-i)
Last OK:
changeset:53de36ab95d1
First Bad
changeset:c4d628fe6be2

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=53de36ab95d1&tochange=c4d628fe6be2

Contains Bug 597525 and Bug 841001

Comment 1

[Mounir Lamouri (:mounir)]

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.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Testcase

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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)?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Mounir Lamouri (:mounir)]

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

Comment 6

[Mounir Lamouri (:mounir)]

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

Comment 7

[Mounir Lamouri (:mounir)]

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

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Mounir Lamouri (:mounir)]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Could we just back out bug 597525?

Comment 12

[Mounir Lamouri (:mounir)]

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

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We need some improvement of the cloneNode code here. Surely textarea's aren't the only elements that rely nsIMutationObserver notifications.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

But whatever improvement we do, it must happen in m-c, not in Aurora. So could we backout 
bug 597525 for now?

Comment 15

[bhavana bajaj [:bajaj]]

:mounir, can you please help prepare the back out , seems like the plan going forward per comment #14 .Correct ?

Comment 16

[bhavana bajaj [:bajaj]]

Spoke offline with :Mounir on this and he is going to backout bug 597525, hence no need to track this

Comment 18

[Mounir Lamouri (:mounir)]

Bug 597525 has been backed out on aurora and central.

Comment 19

[Toshihiro Yamada]

OK, confirmed fixed by backout of bug 597525.

Thank you for your great works.

Comment 20

[Mitza]

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