Closed Bug 288821 Opened 16 years ago Closed 16 years ago

Select cleared on reload.

Categories

(Core :: Layout: Form Controls, defect)

x86
All
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: hhschwab, Assigned: mats)

References

()

Details

(Keywords: regression, testcase)

Attachments

(5 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050329
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050329

You are looking at this bugzilla page. Just do a reload, and look at the OS:
Field. You shortly will see "Win98", and then it is cleared. 
When I change tabs, come back to a bug, sometimes more than OS is missing:
I´ve got Bug 285730 in loaded in 2 tabs side by side, one is miising the
contents of Component:, and to the right: OS: Version: Severity: Target Milestone:
correctly shwon are Hardware: and Priority:

I´m setting Severity to Major, as I don´t want to see this behaviour in the
wild. Don´t mind, as long as it is seen only here.
I can´t see this on a local copy of a bug page, so I can´t produce a testcase. 


Reproducible: Always

Steps to Reproduce:
1. Scroll to the top of the bug
2. Reload, watch the contents of field OS:
3. Shift-Reload, watch the contents of field OS:

Actual Results:  
on reload, the contents of some selects is shortly shown, then vanished.
Mostly seen on OS:

Expected Results:  
render as specified in the Select fields.
Attached file reduced testcase (obsolete) —
testcase stripped down from this page, added <base
href="https://bugzilla.mozilla.org/"> to get it working locally.

Should show 
Product: Core	    OS: Windows 98

On reload Windows98 and/or Core are shown shortly, then cleared.
That doesn´t happen all the time, maybe multiple reloads are required.
I´m seeing the bug always on the original bugpage, and often on the testcase.
Attached file minimized testcase (obsolete) —
Part of the  source:

<base href="https://bugzilla.mozilla.org/">
<link href="skins/standard/global.css" rel="stylesheet" type="text/css">
<link href="skins/custom/global.css" rel="stylesheet" type="text/css">
</head>

<body bgcolor="#ffffff"> <table cellspacing="1" cellpadding="1" border="0">
    <tr>
      <td align="right"><b><u>P</u>roduct:</b></td>
	  <td>
		<label for="product" accesskey="p">
			<select name="product" id="product">
	  <option value="Composer">Composer
	  </option>
	  <option value="Core" selected>Core
	  </option>
	  <option value="Directory">Directory
	   </option>
      </select>
    </label>
  </td>   

CSS must be external, for getting a delay, I assume.
Previous testcase didn´t have <label... but did have </label>


I´m seeing the bug on Reload, always on the bugzilla page, often, not always on
the testcase. Computer is a slow Celeron 333 128 MB RAM.
Attachment #179450 - Attachment is obsolete: true
So... both the bugzilla page and the testcase work fine here with a
2005-03-29-05 Linux trunk SeaMonkey nightly with no extensions.
Attached file Testcase #3
Forcing a reflow triggers it for me (on Reload).
Attachment #179454 - Attachment is obsolete: true
This is a pretty bad regression I would say, suggestions?
Assignee: nobody → mats.palmgren
Flags: blocking1.8b2?
OS: Windows 98 → All
Any idea why this is happening?  It looks like either we're failing to call
RedisplayText or we're failing to reflow afterward.  Maybe we need to mark the
select dirty too, not just the display frame?
Attached file textcase #3 minimized
removed style and table, so only one select remains:

<body bgcolor="#ffffff">
      <select name="op_sys" id="op_sys">  
	  <option value="Windows 98" selected>Windows 98</option>
<script type="text/javascript" language="javascript">var x =
window.scroll(0,1000);</script>
	  <option value="Windows ME">Windows ME</option>
      </select>
</body>
We get two calls to RedisplayText() in quick succession:

[0x877d788] >>>>>>> RedisplayText: aIndex=-1 mDisplayedIndex=0
[0x877d788] RedisplayText: current="1" new=""
[0x877d788] RedisplayText: shouldSetValue=1
[0x877d788] creating RedisplayTextEvent=0x8787dd8 ""
posting RedisplayTextEvent on 0x877d788
[0x877d788] <<<<<<< RedisplayText: aIndex=-1 mDisplayedIndex=-1
[0x877d788] >>>>>>> RedisplayText: aIndex=0 mDisplayedIndex=-1
[0x877d788] RedisplayText: current="1" new="1"
[0x877d788] RedisplayText: shouldSetValue=0
[0x877d788] <<<<<<< RedisplayText: aIndex=0 mDisplayedIndex=0
Document file:///usr/local/test/288821.html loaded successfully
[0x877d788] ####### Handle event RedisplayTextEvent=0x8787dd8  ""
[0x877d788] deleting RedisplayTextEvent aEvent=0x8787dd8


The problem is that the first event has not been handled when the second
call is made and there is an optimization of sorts that peeks at current
value and ignores the request if it's the same as the requested value.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.311&root=/cvsroot&mark=1813-1825#1786

I think we should just remove that block. I guess we could count the number
of outstanding events and ignore them in RedisplayTextEvent::HandleEvent if
the number > 1 if we really care... but I don't think so - RedisplayText()
does not seem like it's called that often and it doesn't seem like a heavy
operation...
Attached patch Patch rev. 1Splinter Review
Attachment #179487 - Flags: superreview?(bzbarsky)
Attachment #179487 - Flags: review?(bzbarsky)
Hmm... Don't we want to just cancel posted redisplay events if a redisplay comes
in while they're posted?  Otherwise it seems like we'll end up with the wrong
index when all is said and done...

That said, if this is indeed called rarely (for example, not called lots of
times during page parsing as options are appended to the select), then I can see
removing the optimization.
(In reply to comment #12)
> Otherwise it seems like we'll end up with the wrong
> index when all is said and done...

mDisplayedIndex is updated directly in RedisplayText(), it's only the
"refreshing" of the text frame that is delayed with the event and what text
to display is carried with the event so the update is not index dependent.
I don't see how we could end up with the wrong index...

I tested it on a <select> with many <option>s (I took the testcase in
bug 252158 and changed them to size="1").
On SHIFT-Reload I get zero calls to RedisplayText.
On Reload I get 6 calls to RedisplayText (there are 5 <select>s)

I also tried it on this Bugzilla page.
SHIFT-Reload: zero calls.
Reload: 9 calls (there are 13 <select size=1> here and 9 of them have an
<option selected> as far as I can see).

Comment on attachment 179487 [details] [diff] [review]
Patch rev. 1

r+sr=bzbarsky
Attachment #179487 - Flags: superreview?(bzbarsky)
Attachment #179487 - Flags: superreview+
Attachment #179487 - Flags: review?(bzbarsky)
Attachment #179487 - Flags: review+
Actually, I think we still have one problem.  PLEvents are not guaranteed to be
delivered in the order posted if you always post to the current event queue
(because the event posted later may get posted to a nested event queue).  So I
think we want to revokeEvents any time we're posting an event here, so that we
only have one event in flight at any given time...
(In reply to comment #15)
> PLEvents are not guaranteed to be delivered in the order posted

Wow, how nasty, I didn't know that.
Unfortunately I've already checked in Patch rev. 1 (2005-04-03 12:58 PDT).

> So I think we want to revokeEvents any time we're posting an event here

Ok, I will do a followup patch for that..

Trace output with this patch:

[0x8765f9c] >>>>>>> RedisplayText: aIndex=-1 mDisplayedIndex=0
[0x8765f9c] creating RedisplayTextEvent=0x8760a58 ""
[0x8765f9c] posting RedisplayTextEvent 0x8760a58 ""
[0x8765f9c] <<<<<<< RedisplayText: aIndex=-1 mDisplayedIndex=-1
[0x8765f9c] >>>>>>> RedisplayText: aIndex=0 mDisplayedIndex=-1
[0x8765f9c] creating RedisplayTextEvent=0x8760a98 "1"
[0x8765f9c] RevokeEvents
[0x8765f9c] deleting RedisplayTextEvent aEvent=0x8760a58
[0x8765f9c] posting RedisplayTextEvent 0x8760a98 "1"
[0x8765f9c] <<<<<<< RedisplayText: aIndex=0 mDisplayedIndex=0
Document file:///usr/local/test/288821.html loaded successfully
[0x8765f9c] ####### Handle event RedisplayTextEvent=0x8760a98  "1"
[0x8765f9c] deleting RedisplayTextEvent aEvent=0x8760a98
Attachment #179512 - Flags: superreview?(bzbarsky)
Attachment #179512 - Flags: review?(bzbarsky)
Comment on attachment 179512 [details] [diff] [review]
Patch B rev. 1 (additional patch)

r+sr=bzbarsky.	Looks great.
Attachment #179512 - Flags: superreview?(bzbarsky)
Attachment #179512 - Flags: superreview+
Attachment #179512 - Flags: review?(bzbarsky)
Attachment #179512 - Flags: review+
Hermann, thanks for the heads up, if you could verify the fix on that slow
Windows box that would be great. Thanks again.

Patch B rev. 1 (additional patch) checked in 2005-04-03 15:16 PDT.

-> FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: blocking1.8b2?
Resolution: --- → FIXED
Boris, I'm a bit curious under which circumstances a nested event queue is
created?
Any time a modal dialog is posed, for example... So say you put up an alert in
window A while an event is posed for a <select> in window B; then event won't
fire until the alert comes down.
On Win98SE, Athlon XP1600, using testcase 3, I always can see the regression in
the regressed builds, and I could see it on the normal bugpage sometimes, when I
did a reload while doing a fast download via DSL.

tested regressed: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050401

tested working: tinderbox build BuildID 2005040322
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.