35340 - wysiwyg: urls - pages created by document.write not added to session history

Status: RESOLVED FIXED

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

Description

[chriss]

NETSCAPE 6.0 BUG REPORT

DATE: 04/06/00

DESCRIPTION:
Web Integration Services personnel (Leyla Oswald, Jim Garza, Josh Metz) have 
detected the following bug with the new Netscape 6.0 browser and how it 
navigates to Merchant web sites from the Shop@ channels.

Across all Shop@Brands (AOL, AOL.com, Netcenter, CompuServe, Digital Cities), 
the Shop@ pages hosted by AOL load with no problem.  

When a Partner web site is called, the browser displays a blank page and does 
not load the called web page.  Hitting the BACK button sends the user two 
pages back instead of the previous page.


OPERATING SYSTEMS:
The symptoms noted in this email were found on both the Windows and MAC 
versions of Netscape 6.0 with the TYPICAL installation chosen.


RECREATION INSTRUCTIONS:

1.  Starting from:
http://webcenters.netscape.com/shopping/home.html

2.  Go to:
Apparel:
http://info.netscape.com/fwd/shicatgry/http://shop.netscape.com/apparel/main.a
dp

3.  Choose a Merchant Partner promotion such as the GAP. 

4.  Once selected, the new page does not load. 

5.  If you select BACK, you are directed to the main shopping page, instead 
of the previous Apparel page.

6.  This can be recreated consistently through the Shop@ department and 
Commerce Center pages across all Brands.


RECREATION ANOMALIES:

There are some anomalies to this recreation test:

-  Infrequently, after selecting a site and getting the blank screen, when 
going back you return to the correct shopping page.

- It also appears that the sites that are working seem to be Silver 
placements for the most part.  Silver placements can be found as hyperlinks 
at the bottom of Department level pages (e.g. 
shop.netscape.com/apparel/mens.adp).

- From all brands, links to Partner web sites from the Chic Simple catalogue 
(http://shop.netscape.com/ams/clickThruRedirect.adp?55004543,4775169,../chicsi
mple/feature/index.adp) function properly.

Comment 1

[chriss]

adding beta 2 keyword fpr PDT review

Comment 2

[Asa Dotzler [:asa]]

[e-mail] have you tested this with current Mozilla nightly builds?

Comment 3

[Tracy Poff]

Added self to cc list.

Comment 4

[chriss]

with the Sunday tip, I get a page that says to use Netscape or IE when I click 
on a partner link.
When I click on the back button, I don't go back to the originating page.

Comment 5

[Asa Dotzler [:asa]]

-> Networking 

Comment 6

[Warren Harris]

Sounds like a bug in the history mechanism. => radha

Comment 7

[Phil Peterson]

[nsbeta2+]

Comment 8

[don]

Move to M18 target milestone.

Comment 9

[Chris McAfee]

I need help reproducing this, sent mail to chriss.

Comment 10

[Radha on family leave (not reading bugmail)]

Chris, thanks for helping me out.

Comment 11

[Michael La Guardia]

I just tried this with today's build, and though I was able to see the page
clicked to from the apparel page, I crashed going back.  Here is the talkback
report:

http://cyclone/reports/incidenttemplate.CFM?reportID=1065&style=0&tc=1&cp=1&ck1=SNub+trigger+event+time&cd1=2000%2F07%2F10&bbid=13934057

Comment 12

[Michael La Guardia]

oops. Removed cc incorrectly.

Comment 13

[Michael La Guardia]

Adding Crash keyword.  I just tried it again and verified that I do indeed crash
using 2000071008

Comment 14

[Jay Patel [:jay]]

adding topcrash keyword.  this crash has made it to the top10 talkback crash 
list today.  here is some info, probably michael's entries:

nsGenericElement::HandleDOMEvent ca67d1e5
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/src/nsGenericEle
ment.cpp line 1370
        Build: 2000071009 CrashDate: 2000-07-10 UptimeMinutes: 2  Total: 2 
        OS: Windows NT  4.0 build 1381
        URL: 
        Comment: testing bug 35340 (going back from gap to apparel
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=13934057

    nsGenericElement::HandleDOMEvent ca67d1e5
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/src/nsGenericEle
ment.cpp line 1370
        Build: 2000071009 CrashDate: 2000-07-10 UptimeMinutes: 7  Total: 10 
        OS: Windows NT  4.0 build 1381
        URL: 
        Comment: Yep
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=13934323

Comment 15

[Michael La Guardia]

Using 2000071220 on Win98 I can no longer get this to crash, although I'm still
seeing the skip-a-page back behavior.  I'm following the steps outlined in the
bug.  I go to the Shop@ pages.  I click on the "Apparel" link in the upper left
of the page.  I click on the Gap ad in the page that comes up.  When the Gap
page loads, I click back.  It was at this point that I was crashing before,
though I don't see that problem now.  Now, instead of going back to the Apparel
page, I'm taken back to the original Shop@ page.

I don't see this as a huge problem any more, and I wouldn't recommend holding
beta for it.

Removing crash keyword.  Chris, if you agree with my beta assesment, could you
go ahead and change this to nsbeta2- in the status whiteboard?

Thanks.

Comment 16

[Chris McAfee]

yes, I see the page skip now.

Comment 17

[Chris McAfee]

agree with michaell, nsbeta2-, over to radha

Comment 18

[johng]

nav triage team:
need to fix redirecting

Comment 19

[Radha on family leave (not reading bugmail)]

This page is very unique. Even 4.7 misbehaves on back and forward in this page. 

Here's what's going on:
1) After going to the second page(apparel or accessories section)  you click on 
a partner promotion page. At this time, the main content area replaces itself 
with 2 frames. This is *not* equivalent to going to a new frame set page. No 
loadURI() is called on the toplevel main content area. So, this screws up the 
normal assumptions docshell makes about loading pages with frameset. Because of 
this, the promotion partner page(GAP or ashford) does not get added to SH 
(though it is loaded). when you click back, SH just takes you to the page 1) 
because as for as SH is concerned, page 2) is the current page.

4.7 skips page 3)(the partner's site)  while going back and forward instead of 
skipping page 2)

Comment 20

[Daniel Veditz [:dveditz]]

Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so 
the queries don't get all screwed up.

Comment 21

[Daniel Veditz [:dveditz]]

Adding nsbeta2 keyword to bugs with nsbeta2 triage value in status field so the 
queries don't get screwed up

Comment 22

[Radha on family leave (not reading bugmail)]

This page is really complex. 4.7 also misbehaves in this page. I'm working on a 
fix for it. But the fix will bring seamonkey's behavior similar to 4.7's 
misbehavior. I do not believe this can be completely fixed. Waiting for rpotts 
to provide more feedback.

Comment 23

[Radha on family leave (not reading bugmail)]

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

Comment 24

[Radha on family leave (not reading bugmail)]

Discussed this withe rick today. This needs more work in layout and probably 
cache area. Rick and Nishseeth have set up a meeting to discuss this with 
Brendan and vidur. Details to emerge later

Comment 25

[Phil Peterson]

PDT marking P5 since page misbehaves in 4.7 and no progress in three weeks

Comment 26

[Radha on family leave (not reading bugmail)]

Eric, this is the bug related to wyciwyg:// protocol. I'm assigning this to you, 
since you will be doing most of the work.

Comment 27

[Eric Pollmann]

The implementation is now done, testing begins...

Comment 28

[Eric Pollmann]

Due to the P5 rating of this bug and a lack of time, I'm not going to get this
in for beta3.  If anyone would like to pick up where I left off, I'm going to
attach my work (based on the framework Gagan was kind enough to provide)

The file is a gzip'd tar, and the patch can be applied like so:
cd /builds/$USER/mozilla
tar -zxvf wyciwyg.tgz
patch -p0 < layout.patch
patch -p0 < netwerk.patch

On Windows, winzip should be happy to explode the tgz for ya.

The remaining problems are:
1) I used synchronous write methods in my implementation of nsWyciwygChannel. 
As Gagan predicted, they are a little sketchy.  For example, when someone loads
this document:
<html>
 <body onload="document.open();
               document.write('<HTML>');
               document.write('foo');
               document.write('</HTML>');
               document.close()">
 </body>
</html>

The wyciwyg cache file (for some reason it's going to disk) contains this:
<HTMfoo</HT

I also tried doing a flush after every write in
nsHTMLDocument::ScriptWriteCommon - no dice.

2) Reading the file from cache doesn't work (not investigated)

3) The session history drop-down entry is showing as "wyciwyg://0/http://..."
instead of the original URL.  Guessing this has something to do with session
history not checking the original URL, but I really don't know - this seems
minor.

4) The entry in session history for the original document is also showing up as
the wyciwyg URL.  I am probably doing something wrong in
nsHTMLDocument::OpenCommon - not investigated.

Comment 29

[Eric Pollmann]

Attachment: wyciwyg.tar.gz - first pass implementation

Comment 30

[Eric Pollmann]

Attachment: same as above (with diff -u for readability)

Comment 31

[Eric Pollmann]

Removing beta3+ - I won't be able to get to this one in time.

Comment 32

[Kevin McCluskey (gone)]

Marking nsbeta3-.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

This bug is closely related (or possibly identical to) bug 46828.  This (or at
least the "skips one going back" behavior it shares with 46828) shows up in a
LOT of pages that work correctly for ns4.7.

I realize that Eric can't make it for nsbeta3.  Perhaps someone else can pitch
in?  In any case, due to the frequency this happens, and the problems it causes
(and that a dependant bug is marked nsbeta3 and severity critical w/ 7 votes)
I'm upping the severity on this one.  I would suggest that it also be upped from
a P5 to get it on more people's radar, but I won't touch it myself.  OS -> All,
Platform -> All

Comment 34

[johng]

cc'ing chrisn, greggl - guys, this is not getting fixed on the client side so
the server side folks at Netscape Shopping better fix it on their side or push
back on client.

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Adding RTM
This is really a bug in the browser - getting one site (netscape shopping) to
fix it isn't going to help us much.
We MUST address this for RTM (IMHO, of course).
This is a lot higher priority than 5.  See bug 46828 for examples - this breaks
all sorts of framed sites (most framed sites).

Comment 36

[Eric Pollmann]

adding keywords to (hopefully) improve findability of this bug.

Comment 37

[Radha on family leave (not reading bugmail)]

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

Comment 38

[Radha on family leave (not reading bugmail)]

resummarising for better tracking. 

Comment 39

[Kevin McCluskey (gone)]

Since this doesn't work properly on Mozilla or 4.7 it sounds like they have been
coding to an assumption about I.E. They really need to change the page so it's
friendly to Mozilla and 4.7. Based on this, marking it rtm-.

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

I should note that other bugs that depend on this one work perfectly correctly
in ns4.7 and ie, and fail only in mozilla.  Not fixing this one will inhibit
working on those if they really are dependant.

The problem in the testcase that Eric posted is not an IE-ism, it's a real
problem with mozilla.

Comment 41

[Radha on family leave (not reading bugmail)]

Post RTM, we need to take care of this for the next release. Removing old
keywordsetc.. and upping priority.  Netcenter webmail seems to use this too.

Comment 42

[Eric Pollmann]

Made some progress on this:

a) Merged to tip (changed PROGID to CONTRACTID in a few places)
b) Update to makefile.win's so it builds on Windows
c) Fixed the problems 1), 2), and 4) that I listed above

This leaves the following problems:
3) The string listed in the session history drop-down is a wyciwyg: url, not the
page title, the original url, or something descriptive like "(dynamic page)"
5) The fix does not work if the content that is written out is long, and
document.close() is not called.  I think this is because the cache entry is
still marked as "update in progress".  Perhaps going to a new URL should unmark
the entry we are leaving?

Tested, and this fixes Bug 46701
Should also fix Bug 54482
Does not fix the Netcenter URL above because it does not call document.close().

I'll attach the latest patch after some cleanup later...

Comment 43

[Eric Pollmann]

updating milestone, keywords, cc, summary, status...

Comment 44

[Eric Pollmann]

Fixing my milestone faux pas...  Thanks Eugene.  :)

Comment 45

[Viswanath Ramachandran]

nav triage team: nsbeta1+ for checking in fix to 1,2,4. Nice to have 3 but
better to at least have 1,2,3 and track 4 separately. 

Comment 46

[Viswanath Ramachandran]

eric, how's this going - will we have it for mozilla0.9? thanks, Vishy

Comment 47

[Radha on family leave (not reading bugmail)]

Eric: I think the third problem is because in nsISHEntry::GetTitle() if the
title for a page is empty, the URI is used a title.  THis change was made so
that directory listings, ftp urls etc.. won't show up empty in the menu.  I can
probably change that code to show something more decent for wyciwyg:// urls. If
the functionality is otehrwise working, can you checkin your chnages? 

Bugs in my list that depend on this include bug 58756 and 60946.

Thanks,

Comment 48

[Eric Pollmann]

Radha, thanks - with a small change in the SH code you mention, I go the title
working now too - only have issue 5) to deal with - people who forget to
document.close() are not going to be happy (and there are lots of cases like
this on the web.  I still need to investigate the severity of 5) and possible
workarounds so that we can decide if checking this in with that issue is still
okay...

Comment 49

[Radha on family leave (not reading bugmail)]

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

Comment 50

[Radha on family leave (not reading bugmail)]

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

Comment 51

[Eric Pollmann]

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

Comment 52

[chris hofmann]

eric is no longer around.  
we need some more time to figure out who can own this bug.
anybody have ideas?

unlikley this will make 0.9.4

Comment 53

[Randell Jesup [:jesup] (needinfo me)]

We need to find a home for this one.  This is P1 Major with a partial fix
bit-rotting...  Reassigning this bug to Asa for reassignment, since no one has
stepped up to take it.

Comment 54

[Asa Dotzler [:asa]]

radha, maybe? I'm the worst possible owner for this since I'm not even a developer.

Comment 55

[Radha on family leave (not reading bugmail)]

I'll see what I can do. May be for 1.0, as I'm not very familiar with the layout
code.

Comment 56

[Radha on family leave (not reading bugmail)]

testcase from bug 96215:

<HTML>
<HEAD>
<TITLE>PAGE1</TITLE>

<script type="text/javascript">
function secondpage()
{
  document.open();
  document.write("<html>");
  document.write("<head><title>PAGE2</title></head>");
  document.write("<body>Second Page</body>");
  document.write("</html>");
  document.close();
}
</script>
</head>
<body>

<form>
<input type="button"  onclick="secondpage()" value="Go to Second Page">
</form>

</BODY>
</HTML>
This page isn't stored in session history when "Go to Second Page"
button is clicked.

Comment 57

[Radha on family leave (not reading bugmail)]

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

Comment 58

[timeless]

the testcase isn't valid.
1.
</body>"); should be executed by the html handler which causes the script to 
generate errors and fail to behave as intended.
2.
  document.close();
iirc this doesn't do what authors think it does.
alternative suggestions include using "+" or a regexp

<HTML>
<HEAD>
<TITLE>PAGE1</TITLE>

<script type="application/x-javascript">
function secondpage()
{
  var ohtml=
"<html><head><title>PAGE2<#title><#head><body>Second Page<#body><#html>";
  ohtml=ohtml.replace(/#/g,'/');
  document.open();
  document.write(ohtml);
}
</script>
</head>
<body>

<form>
<input type="button"  onclick="secondpage()" value="Go to Second Page">
</form>

</BODY>
</HTML>

i've stuck the testcase into the url field. fwiw old urls can be found by 
clicking the bug activity link

amusingly, i just triggered
Error: uncaught exception: Permission denied to call method HTMLDocument.write

I suppose to use my testcase i'd have to disable that exception...

Comment 59

[Radha on family leave (not reading bugmail)]

I have been working on this for a while and have implemented the required
feature. I have tested my code with some of the testcases attached to this bug.
I would like a real world site probably off of netcenter to test my code. Can
some one point me to one?

Comment 60

[Radha on family leave (not reading bugmail)]

Attachment: Patch to implement the feature. windows .zip file

Comment 61

[Radha on family leave (not reading bugmail)]

Attachment: patch v 1.1. Missed a file in the previous patch

Comment 62

[Tom Everingham]

qa -> claudius

Comment 63

[Darin Fisher]

some review comments:

1) nsWyciwygChannel seems to have mixed line breaks.  some with \n some with
\r\n.  i suspect this might cause problems when you try to land this patch.

2) looks like nsWyciwygChannel has a mCallbacks member variable that it does not
use.  you have made nsWyciwygChannel implement nsIInterfaceRequestor, and you
pass |this| as the cache transport's notification callbacks.  but,
nsWyciwygChannel:: GetInterface should call mCallbacks->GetInterface.  this is
done to enable status and progress messages when reading from the cache.  you
should implement nsIProgressEventSink, so you can properly forward the
nsWyciwygChannel object as the nsIRequest object in the nsIProgressEventSink
methods.  see nsHttpChannel for an example of this.

3) looks like you aren't using a consistent brace style.  you have 
    
   foo() {
   }

and 

   foo()
   {
   }

please pick one and stick with it.

4) use NS_INIT_ISUPPORTS() instead of NS_INIT_REFCNT()

5) no reason for a nsWyciwygChannel::Create method.  the protocol handler could
just use |new| to instantiate a nsWyciwygChannel object.

6) nsWyciwygProtocolHandler::Create could be replaced with a
NS_GENERIC_FACTORY_CONSTRUCTOR(nsWyciwygProtocolHandler) line in nsNetModule2.cpp

7) also, i'm not sure that this protocol handler belongs in necko.  it seems
like something that is layout specific, and should probably live elsewhere.  i'm
not sure where though.  if it is to be included in necko, then i think necko2 is
more appropriate.

8) indentation in nsWyciwygChannel seems very inconsistent.  please be consistent.

9) in nsWyciwygChannel::OnStopRequest you capture rv's, but you don't do
anything with them... not that there is anything to do at that point.  but, you
should either add some NS_ASSERTION or not bother capturing the rv's.

10) this section of code should be condensed:

  if (aWriteAccess)
    rv = cacheSession->OpenCacheEntry(aCacheKey, nsICache::ACCESS_WRITE,   
                                      PR_FALSE, getter_AddRefs(mCacheEntry));
  else
    rv = cacheSession->OpenCacheEntry(aCacheKey, nsICache::ACCESS_READ, 
                                      PR_FALSE, getter_AddRefs(mCacheEntry));

how about this instead:

  nsCacheAccessMode accessMode = (aWriteAccess ? nsICache::ACCESS_WRITE
                                               : nsICache::ACCESS_READ);
  rv = cacheSession->OpenCacheEntry(aCacheKey, accessMode, PR_FALSE,
                                    getter_AddRefs(mCacheEntry);

or how about changing the second parameter to OpenCacheEntry from PRBool to
nsCacheAccessMode?

11) in the layout patch, you seem to be making an unnecessary buffer copy:

 +  script = ToNewCString(text);
 +  // Save the script in cache
 +  nsCOMPtr<nsIWyciwygChannel> wcwgChannel(
      do_QueryInterface(mDocWriteDummyRequest));
 +  if (wcwgChannel)
 +    wcwgChannel->WriteToCache(script);
 +  nsCRT::free(script);
 
seems like |text.get()| could be used in place of |script| thereby eliminating a
strdup of the data.


anyways, you should make sure and get someone more familiar with the related
docshell and layout code to review those portions.  the necko code is for the
most part OK.  attach another patch, and i'll give it another review.

Comment 64

[Judson Valeski]

just a comment about a home for this handler. I agree that it probably shouldn't
live in necko (it has nothing to do w/ the networking layer). somewhere in
layout does make more sense. 

Comment 65

[Radha on family leave (not reading bugmail)]

Attachment: Patch v 1.2

This patch incorporates suggestions made  by darin and nisheeth.

Comment 66

[Radha on family leave (not reading bugmail)]

Attachment: v 1.3 Missed a file in the previous patch

Comment 67

[Johnny Stenback (:jst)]

sr=jst for the content part, but we'll eventually need to eliminate the
NS_ConvertUCS2toUTF8(text).get() code in nsHTMLDocument::WriteCommon() for
perfornace reasons. There's already a bug on file for doing that (bug 121943).

Comment 68

[Darin Fisher]

some nits:

1- remove extra newline near bottom of nsIWyciwygChannel.idl
2- remove extra newline near bottom of nsWyciwygChannel.h
3- nsWyciwygChannel::mStatus is misaligned in nsWyciwygChannel.h
4- mLoadAttributes is unused.
5- indentation is not entirely consistent in nsWyciwygChannel.cpp -- please
review it carefully to make sure it is consistent.

major:

5- if your channel is canceled or fails, you should Doom() the cache entry, but
it looks like you are not doing that.  with the current patch it would be
possible to end up with a partial cache entry, which i think you're code will
not know how to deal with.  you should probably add a nsresult parameter to
CloseCacheEntry and then if NS_FAILED(that nsresult) mCacheEntry->Doom().  see
nsHttpChannel::CloseCacheEntry as an example.

sorry for not catching (5) earlier.

Comment 69

[Radha on family leave (not reading bugmail)]

Darin:

Both NS4.x and IE manage the throbber well. ie., the throbber doesn't keep
running when there is no document.close(). When reload is hit, IE reloads the
page properly. I do not know if it hits the server. I will probably put my
testcase in a webserver and monitor the server logs. 

But NS 4.x has a mixed behavior. If the page has a document.close() reload on
that page works  properly. If the page does not have a document.write(), it
reloads the previous page.  

Comment 70

[Radha on family leave (not reading bugmail)]

Fix checked into the trunk. 

Comment 71

[Aleksey Nogin]

Two functions in the newly checked in nsWyciwygChannel.cpp can return
uninitialized nsresult values:

content/html/document/src/nsWyciwygChannel.cpp:353
 `nsresult rv' might be used uninitialized in this function

content/html/document/src/nsWyciwygChannel.cpp:563
 `nsresult rv' might be used uninitialized in this function

P.S. Off-topic: there already existed one such problem in content/html/document:

content/html/document/src/nsHTMLDocument.cpp:561
 `nsresult rv' might be used uninitialized in this function

and now there are three of them.

Comment 72

[Radha on family leave (not reading bugmail)]

Please do not modify an existing bug for a different problem. Open a new bug.
Bug manipulation confuses everyone who looks through it. 

Comment 73

[Aleksey Nogin]

Does it really make sense to file a new bug just for a trivial two-line fix to a
code that was just commited a few hours ago as a part of this one???

Comment 74

[Aleksey Nogin]

I've filed bug 122904 on the "may be used uninitialized" issue.

BTW, some info from that bug may also be worth mentioning here:

There is something else (besides the warning) wrong with
nsWyciwygChannel::WriteToCache, the code there is

if (!mCacheTransport && !mCacheOutputStream) {
    ...

    if (mCacheTransport)
      ...;    
  }

Obviously, the second if statement is dead code.

Comment 75

[Susie Wyshak]

Added nsbeta1 and topembed due to Bugscape 9169