293588 - Wrong items are selected in dropdowns

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[José Jeria]

Build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050509
Firefox/1.0+

Since some days back I noticed that the wrong elements are selected in a
dropdown. This has caused me to change all fields in certain bugs when cc:ing
myself on them.

Steps to reproduce:
1. Go to https://bugzilla.mozilla.org/show_bug.cgi?id=293446
2. Now change the url so that it goes to 915 and press enter
3. Notice that its not core/layout tables in the bug, its core /build config
4. Doing a shift + reload shows the correct values

Comment 1

[Peter van der Woude [:Peter6]]

cc bzbarsky

Comment 2

[José Jeria]

Attachment: Screenshot

This works fine in 20050504, broken in 20050505, which makes bug 274784 a
suspect

Note that i haven't enabled the "fastback/bfcache" feature

Comment 3

[Daniel C]

Note that step 2 means change the URL in the location bar, not in the bug...)

After step 2, I saw the expected results, but then I went back, and then forward
again, and voila! The bug was Layout: Build Config...

Sometimes I have to go back and forwards more than once to reproduce. I have
bfcache enabled with 5 viewers.

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is probably the same basic issue as bug 293135 -- the form state is being
placed in the wrong (new) history entry.  Please retest in today's builds; it
should be fixed.

Comment 5

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050510
Firefox/1.0+
I still see this issue with the beast build I've been using since the landing of
bug 293135. Often when I go to a bug the bug is tagged as Firefox/Bookmarks but
when I ctrl-f5 the page the correct Component is shown.

Comment 6

[José Jeria]

Same here. With todays build (20050510 ) i can still reproduce everytime
following the steps in comment 0

Comment 7

[Boris Zbarsky [:bzbarsky]]

bryner, do you have any idea what's up here?

Comment 8

[Brian Ryner (not reading)]

I'm not able to reproduce this problem on any platform, as of 2005-05-11.  Can
you provide any additional details that might help here?  Does the problem
happen with a new profile?

Comment 9

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050511
Firefox/1.0+ ID:2005051111

I still see this bug.

1. Create a new profile.
2. Go to https://bugzilla.mozilla.org/show_bug.cgi?id=293446
3. Accept the secure site dialog ("OK")
4. Manually edit the URL bar so the bug number points to bug 293123
5. Hit Return to load the new bug page in the current tab.
6. Note that bug 293123 says it's in Core / Build Config (Wrong) - Note that
Version: Branch (Wrong)
7. Do a CTRL-F5
8. Note that bug 293123 now says it's in Core / Security: UI (Correct) - Note
that Version: Trunk (Correct)

Comment 10

[Steve England [:stevee]]

FWIW as the page is rendering I see a glimpse of "Component:" set to the right
value (Security:UI) but then when the page finishes it changes to "Build: Config".

Setting my browser.history_expire_days to 0 doesn't change the misbehaviour.

Comment 11

[Boris Zbarsky [:bzbarsky]]

martijn, can you reproduce this by any chance?  If so, could you possibly create
a testcase?  bryner and I are both failing to reproduce this... :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

I was thinking about this...  GetScriptGlobalObject() will return non-null for a
document if it has an mDocumentContainer even if mScriptGlobalObject is null. 
So it may be that UnbindFromTree on the old document is still storing state in
the new document's SHEntry...

Comment 13

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050511
Firefox/1.0+ ID:2005051111

OK, with a new profile and comment 9 repo steps I get the bug.
With a new profile and browser.sessionhistory.max_viewers=0 and the comment 9
repo steps I still get the bug.
But with a new profile and browser.sessionhistory.max_viewers=1 the bug
(incorrect item select in dropdown boxes) goes away.

Comment 14

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050511
Firefox/1.0+ ID:2005051108

i can still repro this with
browser.sessionhistory.max_viewers 0 (bfcache=off!)

i can't with bfcache on (any non-0 value)

Comment 15

[Boris Zbarsky [:bzbarsky]]

http://web.mit.edu/bzbarsky/www/testcases/shistory/formState.html is a minimal
testcase that shows what I think is the problem (with a text input, but the idea
is the same).  Definitely caused by what I said in comment 12...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial patch

This fixes my testcase, but it's not quite right, I think.  See the XXX
comments in the last hunk for details.

Note also that printing messes with the layout history state as well (in
ReflowPrintObject()); I strongly suspect that this shares the docshell with the
non-printing presentation, and I'm not quite sure what the ordering of that
code is with everything else we have going on....

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note that my testcase has security implications that I feel should make fixing
this a blocker for anything we want to release.  I can explain those in this bug
or in a separate bug, as desired...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ok, the XXX-marked code is called from exactly two places:

1)  Printing.
2)  nsDocumentViewer::Hide, if the viewer is non-sticky.

I _think_ we might be ok, since people generally wouldn't be calling Hide() on a
non-current viewer (and if they were, they already had problems)....

bryner, thoughts?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 183357 [details] [diff] [review]
Partial patch

I think this is ok for now; for 1.9 we may want to rethink the entire
ownership/etc model for the layout state...

Comment 20

[Brian Ryner (not reading)]

Comment on attachment 183357 [details] [diff] [review]
Partial patch

If you no longer feel that the XXX-marked code is "definitely a problem with
bfcache on", then please update the comment to reflect that.  Looks good
otherwise, and should fix the Tp regression as well.  Should the check I added
to nsGeneircHTMLFormElement::UnbindFromTree, to make sure there is a
ScriptGlobalObject, be removed?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> then please update the comment to reflect that

Will do.

> Should the check I added
> to nsGeneircHTMLFormElement::UnbindFromTree, to make sure there is a
> ScriptGlobalObject, be removed?

Yes.  Will do that too.

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 183357 [details] [diff] [review]
Partial patch

requesting approval to check this in

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 183429 [details] [diff] [review]
Patch updated to comments

a=brendan, with r+sr propagated.

/be

Comment 25

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050512
Firefox/1.0+ ID:2005051214

I cannot reproduce comment #9 , browser.sessionhistory.max_viewers = 0
I cannot reproduce comment #9 , browser.sessionhistory.max_viewers =5

Comment 26

[Steve England [:stevee]]

Now WFM with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2)
Gecko/20050512 Firefox/1.0+ ID:2005051214 regardless of what
browser.sessionhistory.max_viewers is set to.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fix checked in... No real Tp impact, interestingly enough.