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

RESOLVED FIXED in Firefox 21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: yamadat501, Assigned: mounir)

Tracking

({regression, testcase})

21 Branch
mozilla22
regression, testcase
Points:
---

Firefox Tracking Flags

(firefox20 unaffected, firefox21- fixed, firefox22 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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
Blocks: 597525
Status: UNCONFIRMED → NEW
tracking-firefox21: --- → ?
Component: General → DOM: Core & HTML
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
Created attachment 714787 [details]
Testcase
(Assignee)

Updated

6 years ago
Keywords: testcase

Comment 3

6 years ago
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.
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Comment 8

6 years ago
Created attachment 714949 [details] [diff] [review]
Patch
Attachment #714949 - Flags: review?(bugs)

Comment 9

6 years ago
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+
(Assignee)

Comment 10

6 years ago
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...
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
Could we just back out bug 597525?
Flags: needinfo?(bugs)
(Assignee)

Comment 12

6 years ago
(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.
Flags: needinfo?(jonas)
We need some improvement of the cloneNode code here. Surely textarea's aren't the only elements that rely nsIMutationObserver notifications.
Flags: needinfo?(jonas)
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: ? → -

Updated

6 years ago
Duplicate of this bug: 853803

Updated

6 years ago
status-firefox20: --- → unaffected
status-firefox21: --- → affected
status-firefox22: --- → affected
Version: Trunk → 21 Branch
(Assignee)

Comment 18

6 years ago
Bug 597525 has been backed out on aurora and central.
(Reporter)

Comment 19

6 years ago
OK, confirmed fixed by backout of bug 597525.

Thank you for your great works.

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox21: affected → fixed
status-firefox22: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Comment 20

6 years ago
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.