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)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b10
Tracking Flags:
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)

testcase
771 bytes, text/html
Details
Patch (v1)
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.
Please add test cases as attachments.
Attached file testcase (obsolete) — 
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
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]

    
    

    
  

      
      
      
      

        Attached 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

    
    

    
  

      
      
      
      

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

    
    

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

    
    

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

    
    

    
  

      
      
      
      

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

    
    

    
  
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.
Blocks: 616196

    
    

    
  
Mats, are you going to work on this?

    
    

    
  

      
      
      
      

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

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

    
    

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

    
    

    
  
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.

>+        }

    
    

    
  

      
      
      
      

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

    
    

    
  
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)

    
    

    
  
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)

    
  
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+

    
    

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

    
  
Blocks: 627193

    
    

    
  
(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.
Blocks: 702184

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: