18798 - [WEBSHELL] Wrong behavior restoring frame state after hitting back

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm filing this bug on Browser-General and cc:ing radha and nisheeth.  Could one
of you (Radha and Nisheeth) please either take it or assign it to someone
else...

DESCRIPTION:  I'm seeing a crash in RestoreFrameStateFor when I hit back while a
page that has state to restore is loading.

STEPS TO REPRODUCE:
 * make some bugzilla query at http://bugzilla.mozilla.org/query.cgi
 * let the query load fully
 * click on the link to a bug
 * let the bug load fully
 * comment on the bug (what I did was write some text in the textbox and mark
the bug VERIFIED)
 * submit the form
 * let the page load fully
 * hit back, to go back to the bug
 * WHILE THE PAGE IS LOADING, hit back again.  You need to wait until you start
seeing something show up, or it won't do anything

ACTUAL RESULTS:
 * crash

EXPECTED RESULTS:
 * end up back at bugzilla query

DOES NOT WORK CORRECTLY ON:
 * Linux, mozilla, 1999-11-13-08-M12
 * Linux, mozilla, 1999-11-13, 05:00PDT (my tree, with SUPERWIN branch and some
of my own changes)

ADDITIONAL INFORMATION:

Top of stack trace (from the debug build above):

#0  0x40f83dc5 in RestoreFrameStateFor (aPresContext=0x83ff510,
    aFrame=0x8780f20, aState=0x8835c20) at nsFrameManager.cpp:1449
#1  0x40f83e99 in FrameManager::RestoreFrameState (this=0x86fce38,
    aPresContext=0x83ff510, aFrame=0x8780f20, aState=0x8835c20)
    at nsFrameManager.cpp:1467
#2  0x40fa99a0 in PresShell::ContentAppended (this=0x85c8718,
    aDocument=0x882f588, aContainer=0x882bf98, aNewIndexInContainer=1)
    at nsPresShell.cpp:2067
#3  0x4117c8b7 in nsDocument::ContentAppended (this=0x882f588,
    aContainer=0x882bf98, aNewIndexInContainer=1) at nsDocument.cpp:1512
#4  0x4105f2ce in nsHTMLDocument::ContentAppended (this=0x882f588,
    aContainer=0x882bf98, aNewIndexInContainer=1) at nsHTMLDocument.cpp:1018
#5  0x410578bf in HTMLContentSink::NotifyAppend (this=0x867c070,
    aContainer=0x882bf98, aStartIndex=1) at nsHTMLContentSink.cpp:3488
#6  0x41050a70 in SinkContext::CloseContainer (this=0x8812d68,
    aNode=@0x85ea090) at nsHTMLContentSink.cpp:1249
#7  0x4105449d in HTMLContentSink::CloseContainer (this=0x867c070,
    aNode=@0x85ea090) at nsHTMLContentSink.cpp:2564
#7  0x4105449d in HTMLContentSink::CloseContainer (this=0x867c070,
    aNode=@0x85ea090) at nsHTMLContentSink.cpp:2564
#8  0x412eddfa in CNavDTD::CloseContainer (this=0x8831248, aNode=0x85ea090,
    aTarget=eHTMLTag_select, aClosedByStartTag=0) at CNavDTD.cpp:2832
#9  0x412edf0c in CNavDTD::CloseContainersTo (this=0x8831248, anIndex=6,
    aTarget=eHTMLTag_select, aClosedByStartTag=0) at CNavDTD.cpp:2867
#10 0x412edfc7 in CNavDTD::CloseContainersTo (this=0x8831248,
    aTarget=eHTMLTag_select, aClosedByStartTag=0) at CNavDTD.cpp:2928
#11 0x412ebcd6 in CNavDTD::HandleEndToken (this=0x8831248, aToken=0x83a0a68)
    at CNavDTD.cpp:1553
#12 0x412ea221 in CNavDTD::HandleToken (this=0x8831248, aToken=0x83a0a68,
    aParser=0x887b310) at CNavDTD.cpp:730
#13 0x412e9ca8 in CNavDTD::BuildModel (this=0x8831248, aParser=0x887b310,
    aTokenizer=0x86d3548, anObserver=0x0, aSink=0x867c070) at CNavDTD.cpp:525
#14 0x412f822c in nsParser::BuildModel (this=0x887b310) at nsParser.cpp:1030
#15 0x412f80d8 in nsParser::ResumeParse (this=0x887b310, aDefaultDTD=0x0,
    aIsFinalChunk=0) at nsParser.cpp:956
#16 0x412f8bda in nsParser::OnDataAvailable (this=0x887b310,
    channel=0x8879fe0, aContext=0x0, pIStream=0x87d8e20, sourceOffset=0,
    aLength=6297) at nsParser.cpp:1306

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I can actually get the crash without filling out the form (doesn't really
surprise me).  That is, click on a link from the bug, and hit back twice.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Nominating for [DOGFOOD].

Comment 3

[leger]

Putting on PDT+ radar.

Comment 4

[leger]

Setting QA Contact.

Comment 5

[karnaze (gone)]

Reassigning to Radha.

Comment 6

[don]

OK, Radha says that she thinks she can get this fixed within the existing
Webshell world before she goes on vacation.

Comment 7

[Radha on family leave (not reading bugmail)]

Simple steps to reproduce is,
1) Go to Bugzilla.mozilla.org and query on some bug
2) View the bug(I did http://bugzilla.mozilla.org/show_bug.cgi?id=18804)
3) Click on a link in the bug (I clicked on link to bug 18951)
4) Click back,
5) While 18804 is loading.(You s'd see part of the bug on the screen) click back
6) You will crash. as dbaron has described.

What is happening is, when you click back in step 5) and bugzilla.mozilla.org is
about to be loaded, the old history state for 18804 is deleted and a new history
state is created and saved in SH.  The load of 18804 is cancelled in webshell.

The cancellation of the load of 18804 is done in one thread. But necko starts
receiving data for the original request which was put in, in step 4, and it
therefore starts calling its listeners(layout parser code) in another thread
(which is the stack shown above). The parser/frame code tries to access the
deleted history state and crashes.

I could reproduce this bug only in pages that have lots of form values (like the
bugzilla bug page) which takes longer to save and restore frame state values and
there by coinciding with  necko's timing to call back on its listeners. Clicking
back, back quickly on the bugzilla pages doesn't crash or clicking back,back
slowly in other simple pages doesn't crash.

Proposed solution:
------------------
I think this can be fixed by changing the place where the old history state is
deleted and the new one saved for 18804. This would involve changes in webshell
and SH code both, going thro' a redesign right now.
So, I would like to fix this when the new webshell is functional with the new
SH. That w'd be after Dec 6 when I return from vacation.

Comment 8

[Radha on family leave (not reading bugmail)]

*** Bug 18883 has been marked as a duplicate of this bug. ***

Comment 9

[don]

Move the completion date from "11/26" to "12/17++".  Radha sez:

"Fixing this involves changes in Webshell and Session History both of which are
changing  rapidly.  ... this will be better addressed when the new world is up &
running.  Also, this bug happens only if you do back, back quickly before
allowing the page in between to load completely. Trying to fix this now, will
cause other side effects.  ..."

So we're gonna have to wait until the rest of Webshell lands before we can fix
this.  Do the PDT folks want me to move this to M13?

Comment 10

[leger]

Moving to M13.

Comment 11

[leger]

Adding travis to cc list.  Can we make this just not crash?

Comment 12

[don]

I'll have Bill try to figure out why it's crashing.  (Note that it doesn't crash
all the time.)

Comment 13

[Radha on family leave (not reading bugmail)]

I have described above why it is crashing. It is because, we delete the old
history state and create a new one before we move out to another page. This is
an operation we *have* to do  in order for form values to retain values.  Where
we do this is the question. In the current webshell/SH setup I have to do it
before I start loading the previous page in SH. ( which is where it is done now.
).

It is when you do Back, Back quickly in a page with several form values with out
allowing the page inbetween to load completely, this deletion's timing coincides
with necko's OnDataAvailable() notification for the cancelled load. and we
crash.

Comment 14

[don]

Sounds like there's not an easy hack to avoid the crash?

Comment 15

[don]

With jevering's permission ... I removed the PDT+ status of this bug since it's
now targeted for M13.

Comment 16

[Nisheeth Ranjan]

*** Bug 20355 has been marked as a duplicate of this bug. ***

Comment 17

[Nisheeth Ranjan]

*** Bug 20625 has been marked as a duplicate of this bug. ***

Comment 18

[gerardok]

QA Contact update.

Comment 19

[Steve Dagley]

Per discussion with jar and based on phil's comments putting this bug back on the
[DOGFOOD] radar

Comment 20

[Steve Dagley]

*** Bug 17572 has been marked as a duplicate of this bug. ***

Comment 21

[leger]

Putting on PDT+ radar.

Comment 22

[leger]

Adding [WEBSHELL] to summary line.

Comment 23

[Radha on family leave (not reading bugmail)]

*** Bug 20642 has been marked as a duplicate of this bug. ***

Comment 24

[don]

Removing PDT+ status because it's obvious we're not gonna hold up dogfood for
this.

Comment 25

[leger]

Putting on PDT- radar.  spoke with don, enough of this is working at this time.

Comment 26

[ckritzer (gone)]

Can't check this bug until 21926 is resolved.

Comment 27

[don]

Re-summarized.

Comment 28

[don]

*** Bug 22638 has been marked as a duplicate of this bug. ***

Comment 29

[Radha on family leave (not reading bugmail)]

*** Bug 22188 has been marked as a duplicate of this bug. ***

Comment 30

[Nisheeth Ranjan]

*** Bug 22988 has been marked as a duplicate of this bug. ***

Comment 31

[Bill Law]

Moving all to M14 (for webshell changes that are being deferred till M14 rather
than destabilize M13).

Comment 32

[ckritzer (gone)]

This appears to be working on the 2000-01-19-08 Linux Commercial build.

Comment 33

[ckritzer (gone)]

WORKSFORME on:
   - Linux6 2000-01-20-08 Commercial Build
   - Win98  2000-01-20-08 Commercial Build
   - MacOS86  2000-01-20-08 Commercial Build

Comment 34

[ckritzer (gone)]

Marking VERIFIED.

Comment 35

[Brian Vetter]

I tried to reproduce the problem. While it no longer crashes, you get incorrect 
behavior. For example, I first went to www.microsoft.com.  Then I navigated to 
www.netscape.com. While it was loading the page at netscape.com, I pressed the 
back button.  It no longer crashed, but instead things get a bit weird in the 
page history.  The page for www.microsoft.com is displayed correctly but the 
location edit box contains www.netscape.com.  The next button (and arrow) is 
disabled and you can't navigate to www.netscape.com. If I press the back button 
arrow, I go back to mozilla.org.  If I then push the next button (arrow), it 
takes me to www.netscape.com - it lost that I had gone to www.microsoft.com 
altogether.

Comment 36

[leger]

Clearing WORKSFORME resolution due to reopen.

Comment 37

[Bill Law]

Marking as beta1; this can be fixed with the new session history that will kick 
in when the webshell starts using it.

Comment 38

[leger]

Putting on PDT+ radar for beta1.

Comment 39

[don]

Adding dependency on bug #13374.

Comment 40

[gabriel]

Why not just put a flag in history to say "this page has a saved state" ?

Comment 41

[Phil Peterson]

per PDT, this is good enough progress for beta1. PDT-

Comment 42

[Radha on family leave (not reading bugmail)]

Moving to M15...

Comment 43

[don]

Move to M16 for now ...

Comment 44

[ckritzer (gone)]

nominating for nsbeta2 based on:
 - severity
 - visibility
 - basic functionality broken

Comment 45

[Radha on family leave (not reading bugmail)]

The SH and Webshell code that were involved in the crash are no longer used. Can
someone verify if this still happens?

Comment 46

[Brian Vetter]

I tried it out on M15.  While there is no crash anymore, things get confused
if you rapidly click on the back and forward buttons over and over again. 
The sensitivity of the < and > buttons appear to get "confused".  For example, I 
navigate to www.microsoft.com, then to www.netscape.com.  I hit back, forward, 
back, forward ... repeatedly.  After 3 or 4 iterations, the sensitivity of the 
forward button is disabled.  If I hit back again, things sometimes get better 
(can navigate all the way forward to www.netscape.com again).  However, 
eventually, it "loses" the ability to navigate to www.netscape.com via the 
forward button even though the forward button is enabled.

After trying the whole thing again (I restarted Mozilla), I got slightly 
different behaviour.  It reversed the www.microsoft.com and www.netscape.com in 
the history order.

Comment 47

[Radha on family leave (not reading bugmail)]

I'm going to closr thisout, since the crash doesn't happen. Please open a new 
bug for other Back/forward abnormalities

Comment 48

[gabriel]

"I navigate to www.microsoft.com, then to www.netscape.com.  I hit back,
forward, back, forward ... repeatedly.  After 3 or 4 iterations, the sensitivity
of the forward button is disabled."

- I think the DOJ should be informed about this one...






(Sorry, I couldn't resist it.)

Comment 49

[ckritzer (gone)]

test windows

Comment 50

[ckritzer (gone)]

test linux

Comment 51

[ckritzer (gone)]

test again

Comment 52

[ckritzer (gone)]

test windows again

Comment 53

[ckritzer (gone)]

test linux

Comment 54

[ckritzer (gone)]

test mac

Comment 55

[ckritzer (gone)]

test mac

Comment 56

[ckritzer (gone)]

Marking VERIFIED FIXED on:

- MacOS9 2000-07-06-08-M17 Commercial

- Linux6 2000-07-07-10-M17 Commercial

- Win98  2000-07-07-13-M17 Commercial