527306 - A simple text input.value change will no attribute right value if a div container display style is modified

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Mathieu Pellerin]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2b2pre) Gecko/20091030 Namoroka/3.6b2pre
Build Identifier: 

The summary is probably all wrong but I can't really understand the mechanics behind what happens in the code and why it results in wrong text.value ... The bug revealed itself while using a web application I'm developing on firefox 3.6b1.

I narrowed down the problem and created two test cases.
test-fail.html (bug, div display style change)
test-work.html (no bug, no div display style change)

The problem appears both when jit.content is true or false.

Reproducible: Always

Steps to Reproduce:
1. Open attached 'test-fail.html'
2. Click on edit button next to the first 'Battambang' item
(you can change item text with the edit text box that appeared)
3. Click on edit button next to the second 'Mondolkiri' item
Actual Results:  
When clicking on edit button of a second item, the value in the edit text box is not updated to use the second item's text but keep value of first item.

Expected Results:  
The edit text box value is updated to the content of second item.

(I'm setting severity of bug to critical as the behavior can lead to lose of data if one is not aware of value wrongfully assigned during interface with page)

Comment 1

[Mathieu Pellerin]

Attachment: testcase showing bug

Comment 2

[Mathieu Pellerin]

Attachment: testcase without bug after removing div display style changes

Comment 3

[Ria Klaassen (not reading all bugmail)]

Works: http://hg.mozilla.org/mozilla-central/rev/7b06b19e4af6
Fails: http://hg.mozilla.org/mozilla-central/rev/c6692a8f3f27
So if there has been no sneaky fix and re-regression after 3 Aug 2009, the regression window should be: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b06b19e4af6&tochange=c6692a8f3f27

I see no javascript engine in that range, so I'll change it to layout form controls, do you agree?

Comment 4

[Mathieu Pellerin]

Ria, sounds like a logical thing to do. Could you change bug's status to confirmed?

From the pushlog list you came up with, bug 494546 seems to be a good culprit candidate to start with. Is it possible for someone to back off this change locally and see whether this bug's gone?

Comment 5

[Timothy Nikkel (:tnikkel)]

Doesn't seem to be caused by bug 494546 as effectively disabling the changes of bug 494546 by never doing the insert asynchronously in RecreateFramesForContent still shows this bug.

Comment 6

[Timothy Nikkel (:tnikkel)]

Seems to be caused by bug 506874.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, three relevant-ish warnings when clicking that second button:

WARNING: NS_ENSURE_TRUE(mShell) failed: file /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp, line 1877
WARNING: Somehow not a plaintext editor?: file /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp, line 2661
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp, line 1906

That first rv is ignored.  But weakFrame.IsAlive() is false at:

2660            if (!plaintextEditor || !weakFrame.IsAlive()) {
2661              NS_WARNING("Somehow not a plaintext editor?");
2662              return NS_ERROR_FAILURE;
2663            }

so we return from nsTextControlFrame::SetValue without setting the value.  nsHTMLInputElement::SetValueInternal ignores the error return from SetFormProperty and returns NS_OK, so there's no exception from the script's point of view.

Comment 8

[Boris Zbarsky [:bzbarsky]]

And the stack to the relevant Destroy() call:

#0  nsTextControlFrame::Destroy (this=0x203eff30) at /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp:1109
....
#12 0x11be902d in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x16731730) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsCSSFrameConstructor.cpp:11601
#13 0x11c61781 in PresShell::FlushPendingNotifications (this=0x167312e0, aType=Flush_Style) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsPresShell.cpp:4879
#14 0x11d0f0af in nsTypedSelection::selectFrames (this=0x16743ba0, aPresContext=0x206c8200, aRange=0x1673f120, aFlags=0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:4371
#15 0x11d11ba9 in nsTypedSelection::Clear (this=0x16743ba0, aPresContext=0x206c8200) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:3902
#16 0x11d1657a in nsTypedSelection::Collapse (this=0x16743ba0, aParentNode=0x16743a90, aOffset=0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:5241
#17 0x11d17ad1 in nsFrameSelection::TakeFocus (this=0x16743ac0, aNewFocus=0x16743a90, aContentOffset=0, aContentEndOffset=1, aHint=nsFrameSelection::HINTLEFT, aContinueSelection=0, aMultipleSelection=0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:1864
#18 0x11d180ef in nsFrameSelection::SelectAll (this=0x16743ac0) at /Users/bzbarsky/mozilla/debug/mozilla/layout/generic/nsSelection.cpp:2288
#19 0x11d579cb in nsTextInputSelectionImpl::SelectAll (this=0x16743810) at /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp:930
#20 0x11d59fcc in nsTextControlFrame::SetValue (this=0x203eff30, aValue=@0xbfffc02c) at /Users/bzbarsky/mozilla/debug/mozilla/layout/forms/nsTextControlFrame.cpp:2658

That flush in selectFrames was added in bug 506874.  

Note that nsHTMLInputElement::SetValueInternal has:

1105       // No need to flush here, if there's no frame at this point we
1106       // don't need to force creation of one just to tell it about this
1107       // new value.
1108       formControlFrame = GetFormControlFrame(PR_FALSE);

We could solve that by in fact performing a frames flush if there is a form control frame, but that's the common case.... Is there a way to avoid flushing in selectFrames?  Or avoiding selection operations in the value setting codepath?  I suppose we could just toss up a script blocker around the value set, but does selection _really_ need that frames flush?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

As far as I recall, that flush is not need to fix bug 506874 specifically. I added it because I was writing tests like

<body style="white-space:pre">0123456789
<script>
var t = document.body.firstChild;
var sel = window.getSelection();
sel.collapse(t, 2);
sel.extend(t, 8);
</script>

and I observed that the tests were failing without the flush. The reason is of course that when that script runs, we don't have a frame for the <body> child, so selectFrames doesn't set selection state on any frame, and frame construction does not consult selection, so the text never looks selected. Flushing frames for selectFrames isn't a complete fix, since reframing still loses selection state, but it fixes this simple case. I would be surprised if no authors have encountered it in the wild. A complete fix would mean re-calling selectFrames after each frame construction episode, which suggests worrying performance implications to me, or better still moving selection state completely out of frames into content.

So we can back out the flush, but we'd have to modify all the testcases from bug 506874 to add explicit flush hacks, which is unappetizing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Having selection on the frame tree really sucks here....

I suspect the unappetizing thing is the safe thing to do, though we could make window.getSelection flush... would that solve your test issues?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It would, although of course slight variations (e.g., caching getSelection() in a script in <head> and then using it in later scripts) would remain broken.

Another option would be to only flush in selectFrames when this is not the selection for a form control.

Comment 12

[Boris Zbarsky [:bzbarsky]]

That can still lead to weird behavior, since a flush_frames can destroy presshells, no?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, but we handle that:

  // Re-get shell because the flush might have destroyed it 
  presShell = aPresContext->GetPresShell();
  if (!presShell)
    return NS_OK;

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The only way to always behave non-weirdly is to move all selection state into content, I think.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Don't flush in selectFrames when this is the selection for a form control.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Yeah, but we handle that:

"handle" in the sense of "don't perform the selection", no?

Will designmode iframes that are trying to delete text while there's a pending reframe of the nsSubDocumentFrame around work right with that last patch?  Or are they ok already for some reason?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, and I absolutely agree with comment 14; it's just that if I have to choose between new web-exposed weirdness and old web-exposed weirdness plus a bit of extra weirdness in our tests, I'd prefer the latter, I think.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 411265 [details] [diff] [review]
fix

I suppose you're right. But I hate re-breaking this.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Doesn't make me that happy either... :(

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: safe fix

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 411354 [details] [diff] [review]
safe fix

Looks fine, but why the changes to dynamic-text-1-*?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To work around the problem that Quartz antialiasing causes the test to fail --- antialiased pixels aren't repainted. Bug 476927.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/9c362934e378

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/66c8e0b7ba78