574820 - URL and Placeholders (e.g. "Go to a Web Site") show at same time in Location Bar after closing Print Preview

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of URL bar after coming out of Print Preview mode

Seen while running : Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre and spun off from Bug 574743.

STR:
1. Open a page such as cnn.com
2. File Print Preview
3. Close the dialog in print preview

Receive the attached screenshot. Alice hunted down the regression range for this one. I am not if I filed in this in the correct component or not, but since it is happening in the location bar I thought it was a good start.


Works;
http://hg.mozilla.org/mozilla-central/rev/21f0727c27a6
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre)
Gecko/20100605 Minefield/3.7a5pre ID:20100605111313

Fails:
http://hg.mozilla.org/mozilla-central/rev/2875f9a693ae
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre)
Gecko/20100605 Minefield/3.7a5pre ID:20100605135617

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21f0727c27a6&tochange=2875f9a693ae

Comment 2

[Alice0775 White]

I revert to 2437636244f3 and apply 2875f9a693ae in local build.however the problem still exists.
So, I guess landing patch of Bug 534785 causes the problem.

Comment 5

[(no longer active)]

So the issue here is that we try to SetAttr for the class attribute once with aNotify set to false, and then we try the same thing again for the same class value but this time with aNotify set to true.  nsGenericElement::SetAttr helpfully optimizes out that second SetAttr call because the value is the same.

The fix here would be to pass true as aNotify the first time around, but I really don't like that, because that would mean that we're notifying for no good reason every time except for this specific case.

Boris, is this optimization wrong?  Perhaps we should still go ahead and notify if aNotify is set to true?  Particularly, note that there is no way for the caller to make sure if the notification has really happened or not.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> because that would mean that we're notifying for no good reason every time
> except for this specific case

I'm not sure I follow.  If you change an attribute value, you need to notify, with a _very_ few exceptions.  Why is the code involved one of those exceptions?

> Boris, is this optimization wrong?

No; in fact it's a requirement for correct behavior wrt mutation events.

> Perhaps we should still go ahead and notify if aNotify is set to true? 

We could do that while skipping firing of mutation events, but that would be a performance regression in many cases.  We really want to avoid that.

Comment 8

[Boris Zbarsky [:bzbarsky]]

What's the actual impact of the attached patch?  When is that code executed?

Comment 9

[(no longer active)]

(In reply to comment #7)
> > because that would mean that we're notifying for no good reason every time
> > except for this specific case
> 
> I'm not sure I follow.  If you change an attribute value, you need to notify,
> with a _very_ few exceptions.  Why is the code involved one of those
> exceptions?

Well, nsTextEditorState::SetPlaceholderClass (which is the actual code responsible for calling SetAttr) doesn't check the old attribute value before setting the new one.  I guess it could, but then we'd be ending up checking the value twice when we'll actually end up checking the attribute value.

I avoid notifying if some other code later on would result in notification.  nsTextEditorState::SetPlaceholderClass could be called multiple times before we actually get a chance to display things, that's why I avoid notifying when another notification is going to be processed later on.  But considering the rest of this comment, I guess that was the wrong thing to do, right?

> > Boris, is this optimization wrong?
> 
> No; in fact it's a requirement for correct behavior wrt mutation events.

Hmm, OK, so I was mistakenly assuming that aNotify actually controls whether or not pending notifications are flushed.  But after reading the code carefully, this doesn't seem to be the case.  Right?

If so, I retract my earlier comments!

> > Perhaps we should still go ahead and notify if aNotify is set to true? 
> 
> We could do that while skipping firing of mutation events, but that would be a
> performance regression in many cases.  We really want to avoid that.

Please let me know if I'm getting this wrong, but aNotify controls whether internal mutation observes are being called, and also whether a DOM mutation event is fired.  Is that correct?

(In reply to comment #8)
> What's the actual impact of the attached patch?  When is that code executed?

The code is executed when we try to prepare the editor for a text control which has its editor initialized before when a previous frame was constructed for it, but has that frame removed and a new frame attached since.  That code ends up with SetAttr being called to add the "hidden" class to the placeholder div, but with aNotify set to false, and later on SetPlaceholderClass is called again, which sets the class to the same value, but with aNotify set to true this time.

I'm not sure what code is actually responsible to make sure that the placeholder div which has a |visibility: hidden| style applied to it <http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#181> is actually not considered when the frame tree is being inspected to generate a display list, but that code should be hooked up to a mutation listener, I would suppose.

Comment 10

[(no longer active)]

CCing Mounir, because this could also be interesting to him.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I avoid notifying if some other code later on would result in notification. 

Ah, yeah.  Don't do that; handling notifications are state-dependent, and hence the timing when a notification is sent matters.  In particular, for attribute change notifications the _old_ value of the attribute (which is passed to AttributeWillChange) matters.

> SetPlaceholderClass could be called multiple times before we actually get a
> chance to display things

With different values?

> aNotify actually controls whether or not pending notifications are flushed.

aNotify controls whether we send mutation observer notifications, whether we fire mutation events, and a few other things where we optimize away firing state notifications triggered by a change if aNotify was passed as false.  Fundamentally, !aNotify means "no one anywhere has any state that depends on this thing that I'm changing, and I know it because I just created this object and I'm still setting it up".

> is actually not considered when the frame tree is being inspected to generate
> a display list

The code that constructs those lists...  The mutation observer hookup that updates things and invalidates as needed in this case is PresShell in this case.

Comment 12

[(no longer active)]

(In reply to comment #11)
> > I avoid notifying if some other code later on would result in notification. 
> 
> Ah, yeah.  Don't do that; handling notifications are state-dependent, and hence
> the timing when a notification is sent matters.  In particular, for attribute
> change notifications the _old_ value of the attribute (which is passed to
> AttributeWillChange) matters.

OK, thanks for the clarification.

> > SetPlaceholderClass could be called multiple times before we actually get a
> > chance to display things
> 
> With different values?

Mostly, yes.  The problem is, it's not really straightforward to reason about that in the general case (hence this bug).

> > aNotify actually controls whether or not pending notifications are flushed.
> 
> aNotify controls whether we send mutation observer notifications, whether we
> fire mutation events, and a few other things where we optimize away firing
> state notifications triggered by a change if aNotify was passed as false. 
> Fundamentally, !aNotify means "no one anywhere has any state that depends on
> this thing that I'm changing, and I know it because I just created this object
> and I'm still setting it up".

Would I take a perf hit if I just blindly pass true all the time?

> > is actually not considered when the frame tree is being inspected to generate
> > a display list
> 
> The code that constructs those lists...  The mutation observer hookup that
> updates things and invalidates as needed in this case is PresShell in this
> case.

That explains the root of the problem.  This is an interesting thing to learn, I'll make sure to keep it in mind in the future.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Would I take a perf hit if I just blindly pass true all the time?

In theory, yes.  In practice, that really depends on how many such calls happen, etc...

Comment 14

[(no longer active)]

Hmm, so let's take this patch for now while I try to figure an answer to comment 13.

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/12ea384540e3