Closed
Bug 598726
Opened 14 years ago
Closed 14 years ago
Firefox hangs when transitions are used when blurring a textfield which has a placeholder and a value
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla2.0b10
| Tracking | Status | |
|---|---|---|
| blocking2.0 | --- | final+ |
People
(Reporter: shivk, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
(Keywords: hang, testcase, Whiteboard: [hardblocker](?))
Attachments
(2 files, 4 obsolete files)
|
771 bytes,
text/html
|
Details | |
|
6.01 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
I have the following html (see below).
Typing something in the first field and tabbing out renders the browser and all tabs completely useless (hangs). Eventually, you have no option but to shut down the browser using task manager.
<!DOCTYPE html>
<html>
<head>
<title>Html 5 Form</title>
</head>
<style type="text/css">
body { font-family: Verdana, Arial, sans-serif; font-size: 90%; }
h1, h2, h3, h4 { margin-top: 0px; }
div.row { margin-bottom: 10px; }
div::first-letter { color: black; font-size: 140% }
form {
padding: 20px;
border: 1px solid #cccccc;
border-radius:10px;
-moz-border-radius: 10px;
-webkit-box-shadow:0 0 10px #ccc;
-moz-box-shadow: 0 0 10px #ccc;
box-shadow: 0 0 10px #ccc;
width: 375px;
margin: 20px auto;
background-image: -moz-linear-gradient(top, #ffffff, #f2f2f2);
background-image: -webkit-gradient(linear, left top, left bottom, from(#ffffff), to(#f2f2f2));
}
*:focus{ outline:none; }
input {
border:1px solid #ccc;
font-size:20px;
padding: 5px 10px 5px 10px;
border-radius:10px;
-moz-border-radius: 10px;
-webkit-transition: all 0.5s ease-in-out;
-moz-transition: all 0.5s ease-in-out;
transition: all 0.5s ease-in-out;
}
input[type=text], input[type=email], input[type=url] { width: 350px; }
input.short { width: 150px; }
input:focus, textarea:focus {
-webkit-box-shadow:0 0 10px #ccc;
-moz-box-shadow: 0 0 10px #ccc;
box-shadow: 0 0 5px #ccc;
-webkit-transform: scale(1.05);
-moz-transform: scale(1.05);
transform: scale(1.05);
}
input:not(:focus) {
opacity:0.5;
}
input:required {
outline: none;
background:url("images/required.png") no-repeat 340px 5px;
}
input[class=short]:required {
background:url("images/required.png") no-repeat 140px 5px;
}
input:focus:invalid {
background:url("images/error.png") no-repeat 340px 5px;
}
input[class=short]:focus:invalid {
background:url("images/error.png") no-repeat 140px 5px;
}
input:valid {
background:url("images/ok.png") no-repeat 340px 5px;
}
input[class=short]:valid {
background:url("images/ok.png") no-repeat 140px 5px;
}
input[type=submit] {
opacity:1.0;
background-image: -moz-linear-gradient(top, #ffffff, #cfcfcf);
background-image: -webkit-gradient(linear, left top, left bottom, from(#ffffff), to(#cfcfcf));
}
#titleImage.left { margin-left: 0px; }
#titleImage.right { margin-left: 325px; }
#titleImage { -webkit-transition: margin-left 1s ease-in-out; position: absolute; top: 0px; }
</style>
<body onload="document.getElementById('titleImage').className = 'right';">
<form action="">
<img id="titleImage" src="images/user.png" alt="" />
<h2>Contact Information</h2>
<table>
<tr>
<td>
<div>
<label for="firstName">First Name:</label>
</div>
<div class="row">
<input type="text" name="firstName" id="firstName" class="short" placeholder="John" required autofocus />
</div>
</td>
<td style="padding-left: 20px;">
<div>
<label for="lastName">Last Name:</label>
</div>
<div class="row">
<input type="text" name="lastName" id="lastName" class="short" placeholder="Smith" required />
</div>
</td>
</tr>
</table>
<div>
<label for="emailAddress">Email Address: </label>
</div>
<div class="row">
<input type="email" name="emailAddress" id="emailAddress" placeholder="jsmith@yahoo.com" required />
</div>
<div>
<label for="websiteUrl">Website Url: </label>
</div>
<div class="row">
<input type="url" name="websiteUrl" id="websiteUrl" placeholder="http://mydomain.com" required />
</div>
<input type="submit" value="Submit" />
</form>
</body>
</html>
Reproducible: Always
Steps to Reproduce:
Create an html page using the html provided.
Open the page in FF
Fill in the first field and tab out.
At this point the browser should hang.
Actual Results:
The browser hangs.
Expected Results:
One would expect to go on to the next field in the form
This same form/markup works in webkit and IE 9 beta.
|
||
Comment 1•14 years ago
|
||
Please add test cases as attachments.
|
|
||
Comment 2•14 years ago
|
||
|
|
||
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
Reporter | |
Comment 3•14 years ago
|
||
Sorry Olli, and thanks for doing this for me.
|
||
Comment 4•14 years ago
|
||
I think this should block at least to check what is the origin...
blocking2.0: --- → ?
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
|
|
||
Comment 5•14 years ago
|
||
|
|
||
Updated•14 years ago
|
Attachment #477619 -
Attachment is patch: false
Attachment #477619 -
Attachment mime type: text/plain → text/html
|
||
Comment 6•14 years ago
|
||
So the smaller testcase doesn't have any validation, right?
A profile shows that on each refresh driver timer firing we reframe a text control, basically. That would be consistent with going from having a transform to not having one or vice versa... do we get hit by the fact that we reresolve style once without transforms applied?
|
|
||
Comment 8•14 years ago
|
||
Is the autofocus needed? Is the transition needed? (I assume yes). What other parts can we strip out? Does making the transform scale 1.0 still reproduce the bug?
|
|
||
Updated•14 years ago
|
Component: DOM → Layout
QA Contact: general → layout
Summary: FF hangs on Html 5 field validation → Firefox hangs when transitions are used when blurring a textfield which has a placeholder and a value
|
|
||
Comment 9•14 years ago
|
||
The autofocus has nothing to do with that.
Attachment #477612 -
Attachment is obsolete: true
Attachment #477619 -
Attachment is obsolete: true
|
|
||
Comment 10•14 years ago
|
||
Is the placeholder relevant?
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
|
|
||
Comment 11•14 years ago
|
||
(In reply to comment #10)
> Is the placeholder relevant?
Yes, as noted in the test case, it is. Sorry I should have mentioned that in the comment.
|
|
||
Comment 12•14 years ago
|
||
OK. In that case... I wonder why it's relevant.
blocking2.0: ? → final+
|
||
Comment 13•14 years ago
|
||
Having a placeholder causes a lot of style events, which increases the
chance of hitting the bug. I think it could happen in other cases though.
Here's what happens for the testcases on Linux:
1. nsRefreshDriver::Notify sets mObservingRefreshDriver = NULL to unblock
nsCSSFrameConstructor::PostRestyleEventInternal, then calls
shell->FlushPendingNotifications(Flush_Style)
2. FlushPendingNotifications calls mFrameConstructor->ProcessPendingRestyles()
(under a script blocker).
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4870
3. there's a mPendingRestyles for the text control, while processing
it we do ReResolveStyleContext which calls TryStartingTransition
which transitions -moz-transform, so it posts adds a
mPendingAnimationRestyles event.
4. the restyle leads to reframing the text control,
CreateAnonymousContent calls nsTextEditorState::BindToFrame
which schedules a script runner: "PrepareEditorEvent"
5. ProcessPendingRestyles finds the mPendingAnimationRestyles
which leads to a reframe (again).
6. unwinding back to 2, we remove the script blocker and run
the PrepareEditorEvent (which leads to a recursive invocation
of PresShell::FlushPendingNotifications, which doesn't do any
harm since mPending[Animation]Restyles are empty at this point).
nsTextEditorState::PrepareEditor calls nsTextEditorState::SetValue
leading to nsHTMLInputElement::OnValueChanged which notifies
a content state change (NS_EVENT_STATE_MOZ_PLACEHOLDER), this
adds a new restyle event.
And, this is the crux, since PostRestyleEventInternal isn't
blocked at this point it calls AddStyleFlushObserver(mPresShell).
7. back in PresShell::FlushPendingNotifications we now reach the
2nd call to mFrameConstructor->ProcessPendingRestyles() (line 4891)
8. processing mPendingRestyles finds the -moz-transform has changed
and it leads to 3, 4, 5, 6.
9. back in nsRefreshDriver::Notify we loop and find the same shell
we just processed. goto 1.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#292
|
|
||
Comment 14•14 years ago
|
||
So the key is that a Flush_Style on the presshell ends with it having outstanding restyles, right? I assumed this couldn't happen because ProcessPendingRestyles runs until there aren't any, but hadn't thought about scriptrunners posting restyles...
The double-reframe there is its own special kettle of annoyance I wish we could avoid, but I think the right fix for this bug is to just change the refresh driver to swap or copy the presshell arrays as well, instead of just doing the "grab the last element" thing. David, any objections?
|
|
||
Comment 16•14 years ago
|
||
(In reply to comment #14)
> So the key is that a Flush_Style on the presshell ends with it having
> outstanding restyles, right? I assumed this couldn't happen because
> ProcessPendingRestyles runs until there aren't any, but hadn't thought about
> scriptrunners posting restyles...
I don't know. I don't think this can be easily avoided...
> but I think the right fix for this bug is to just change the refresh
> driver to swap or copy the presshell arrays as well, instead of just doing the
> "grab the last element" thing. David, any objections?
Fwiw, I tried a few different variations of that but it didn't seem
to fix the problem.
After digging some more, I have the feeling the problem has something
to do with nsTransitionManager and how it behaves when the goal of
the transition occurs early due to an unrelated style change.
I noted that the problem goes away if I remove the "return" here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp#607
(but that also breaks layout/style/test/test_transitions.html)
|
|
||
Comment 17•14 years ago
|
||
Just an observation in case it helps finding the right fix:
This fixes the attached testcase (it animates correctly AFAICT) without
breaking test_transitions.html.
|
|
||
Comment 18•14 years ago
|
||
Fwiw, adding this to the testcase makes it not trigger the bug:
input {
-moz-transform: scale(1);
}
|
|
||
Comment 19•14 years ago
|
||
Right, because then the style change for the transform change is a repaint, not a reframe.
|
|
||
Comment 20•14 years ago
|
||
Yes, and without it we hit the ReconstructFrame case:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2029
Assignee: nobody → ehsan
Whiteboard: [hardblocker](?)
|
Assignee | |
Comment 21•14 years ago
|
||
Mats, are you going to work on this?
|
|
Assignee | |
Comment 22•14 years ago
|
||
Actually, bz's idea in comment 14 seems to be the correct fix, which works perfectly. This patch implements this, and goes to great lengths to ensure that the only change in the semantics of which restyles are processed is that the new ones added during the style processing in a refresh driver timer shot will be postponed to the next shot.
This seems to work mostly fine, and I've submitted it to the try server to get the results on all of the platforms/suites as well.
Attachment #479686 -
Attachment is obsolete: true
Attachment #503934 -
Flags: review?(roc)
|
|
Assignee | |
Updated•14 years ago
|
Whiteboard: [hardblocker](?) → [hardblocker](?) [has patch][needs review roc]
|
|
||
Comment 23•14 years ago
|
||
Fwiw, the only reason I grabbed shells to restyle off the end is that it made it easy to write the algorithm that allowed new things to be added. I agree that as aa risk-mitigation measure keeping the order makes sense, but if the code would be simpler if we restyle presshells in a different order we should file a bug to change to that after 2.0.
The other possible concern is that the new code is O(N^2) in number of presshells, but that's usually pretty small....
|
|
Assignee | |
Comment 24•14 years ago
|
||
(In reply to comment #23)
> Fwiw, the only reason I grabbed shells to restyle off the end is that it made
> it easy to write the algorithm that allowed new things to be added. I agree
> that as aa risk-mitigation measure keeping the order makes sense, but if the
> code would be simpler if we restyle presshells in a different order we should
> file a bug to change to that after 2.0.
No, the code wouldn't really be much simpler if we traverse the list in what I would like to call the "grandma order". :-) Which reminds me that I should probably specify here why I did what I did in more detail. I'll do that in a sec.
> The other possible concern is that the new code is O(N^2) in number of
> presshells, but that's usually pretty small....
The order change is not ideal, but I can't think of any way to work around it without changing other things which are relying on this now. See my next comment for more details.
|
|
Assignee | |
Comment 25•14 years ago
|
||
Comment on attachment 503934 [details] [diff] [review]
Patch (v1)
Here is an attempt to explain why I did things the way I did. I'll only do it for the first block, since the other one is basically identical.
>- while (!mStyleFlushObservers.IsEmpty() &&
>- mPresContext && mPresContext->GetPresShell()) {
>- PRUint32 idx = mStyleFlushObservers.Length() - 1;
>- nsCOMPtr<nsIPresShell> shell = mStyleFlushObservers[idx];
>- mStyleFlushObservers.RemoveElementAt(idx);
>- shell->FrameConstructor()->mObservingRefreshDriver = PR_FALSE;
>- shell->FlushPendingNotifications(Flush_Style);
>+ if (mPresContext && mPresContext->GetPresShell()) {
To make sure that we're not trying to start without a prescontext and a presshell.
>+ nsTArray<nsIPresShell*> observers;
>+ observers.AppendElements(mStyleFlushObservers);
I used AppendElements instead of SwapElements to make sure that attempts to add or remove to the observers list during the flushes have the expected effect.
>+ for (PRUint32 j = observers.Length();
>+ j && mPresContext && mPresContext->GetPresShell(); --j) {
The order is reverse to minimize the risk, and if we chose to traverse in the grandma order, this is basically the only place which would change. Hence there isn't much of a readability improvement there.
>+ // Make sure to not process observers which might have been removed
>+ // during previous iterations.
>+ nsIPresShell* shell = observers[j - 1];
This |shell| pointer is really dangerous at this point. Without checking it, it could be pointing into a random place in memory, because the corresponding shell might have gone away by this time. We need to check to make sure that this is an actual valid pointer. See below.
>+ mStyleFlushObservers.RemoveElement(shell);
Remove the shell from the observer list, so that if we need to readd it, we don't assert, and it remains in the observer list for the next refresh timeout.
(While writing this, I realized that I failed to move this code up in the first block. Will post an updated patch next.)
>+ if (!mStyleFlushObservers.Contains(shell))
>+ continue;
If we get here, |shell| is not removed by previous flushes, so we assume that it exists. This assumption is only based on the assumption in the existing code. I don't have good evidence for why it is, but I'm just assuming that the previous code didn't lead to crashes all the time. :-)
>+ NS_ADDREF(shell);
Make sure that the flush doesn't destroy the presshell unexpectedly.
>+ shell->FrameConstructor()->mObservingRefreshDriver = PR_FALSE;
>+ shell->FlushPendingNotifications(Flush_Style);
Do our thing.
>+ NS_RELEASE(shell);
Don't leak the shell. This addref/release simulates nsCOMPtr in the existing code.
>+ }
|
|
Assignee | |
Comment 26•14 years ago
|
||
Attachment #503934 -
Attachment is obsolete: true
Attachment #503994 -
Flags: review?(roc)
Attachment #503934 -
Flags: review?(roc)
Shouldn't Boris be reviewing this?
|
|
Assignee | |
Comment 28•14 years ago
|
||
Comment on attachment 503994 [details] [diff] [review]
Patch (v1)
(In reply to comment #27)
> Shouldn't Boris be reviewing this?
Sure, I'll switch the request to Boris. However I won't do that just now, as I need to address the test failures that I saw on the try server...
Attachment #503994 -
Flags: review?(roc)
|
|
Assignee | |
Comment 29•14 years ago
|
||
Comment on attachment 503994 [details] [diff] [review]
Patch (v1)
Seems like the misplaced RemoveElement call in comment 25 was the cause of all the test failures. So, this patch is ready for review now.
Attachment #503994 -
Flags: review?(bzbarsky)
|
|
Assignee | |
Updated•14 years ago
|
Whiteboard: [hardblocker](?) [has patch][needs review roc] → [hardblocker](?) [has patch][needs review bz]
|
|
||
Comment 30•14 years ago
|
||
Comment on attachment 503994 [details] [diff] [review]
Patch (v1)
r=me, but please file a followup bug for post-2.0 to get rid of those Contains() calls (using observer arrays, say).
Attachment #503994 -
Flags: review?(bzbarsky) → review+
|
|
Assignee | |
Comment 31•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [hardblocker](?) [has patch][needs review bz] → [hardblocker](?)
Target Milestone: --- → mozilla2.0b10
|
|
Assignee | |
Comment 32•14 years ago
|
||
(In reply to comment #30)
> Comment on attachment 503994 [details] [diff] [review]
> Patch (v1)
>
> r=me, but please file a followup bug for post-2.0 to get rid of those
> Contains() calls (using observer arrays, say).
Filed bug 627193.
You need to log in
before you can comment on or make changes to this bug.
Description
•