Firefox hangs when transitions are used when blurring a textfield which has a placeholder and a value

RESOLVED FIXED in mozilla2.0b10

Status

()

defect
--
critical
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: shivk, Assigned: Ehsan)

Tracking

(Blocks 1 bug, {hang, testcase})

Trunk
mozilla2.0b10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker](?))

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

9 years ago
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.
Please add test cases as attachments.
Posted file testcase (obsolete) —
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 3

9 years ago
Sorry Olli, and thanks for doing this for me.
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
Keywords: testcase
Whiteboard: [mounir-g2.0]
Posted file smaller testcase (obsolete) —
Attachment #477619 - Attachment is patch: false
Attachment #477619 - Attachment mime type: text/plain → text/html
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?
Indeed, this has nothing to do with HTML5 Forms.
Whiteboard: [mounir-g2.0]
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?
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
Posted file testcase
The autofocus has nothing to do with that.
Attachment #477612 - Attachment is obsolete: true
Attachment #477619 - Attachment is obsolete: true
Is the placeholder relevant?
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
(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.
OK.  In that case... I wonder why it's relevant.

Comment 13

9 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

Updated

9 years ago
Keywords: hang
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?
Duplicate of this bug: 600597

Comment 16

9 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

9 years ago
Posted patch hack (obsolete) — Splinter Review
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

9 years ago
Fwiw, adding this to the testcase makes it not trigger the bug:
      input {
          -moz-transform: scale(1);
      }
Right, because then the style change for the transform change is a repaint, not a reframe.

Comment 20

9 years ago
Yes, and without it we hit the ReconstructFrame case:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2029

Updated

9 years ago
Blocks: 616196
Whiteboard: [hardblocker](?)
Assignee

Comment 21

9 years ago
Mats, are you going to work on this?
Assignee

Comment 22

9 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
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

9 years ago
Whiteboard: [hardblocker](?) → [hardblocker](?) [has patch][needs review roc]
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

9 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

9 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

9 years ago
Posted patch Patch (v1)Splinter Review
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

9 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

9 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

9 years ago
Whiteboard: [hardblocker](?) [has patch][needs review roc] → [hardblocker](?) [has patch][needs review bz]
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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/fb0393b79575
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [hardblocker](?) [has patch][needs review bz] → [hardblocker](?)
Target Milestone: --- → mozilla2.0b10
Assignee

Updated

9 years ago
Blocks: 627193
Assignee

Comment 32

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