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.

adding beta 2 keyword fpr PDT review

chriss@netscape.com have you tested this with current Mozilla nightly builds?

Added self to cc list.

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.

-> Networking

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

[nsbeta2+]

Move to M18 target milestone.

I need help reproducing this, sent mail to chriss.

Chris, thanks for helping me out.

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

oops. Removed cc incorrectly.

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

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

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.

yes, I see the page skip now.

agree with michaell, nsbeta2-, over to radha

nav triage team: need to fix redirecting

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)

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

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

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.

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

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

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

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

The implementation is now done, testing begins...

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.

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

Marking nsbeta3-.

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

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.

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).

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

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

resummarising for better tracking.

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-.

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.

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.

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...

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

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

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.

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

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,

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...

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

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

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

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

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.

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

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

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.

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

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...

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?

qa -> claudius

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.

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.

This patch incorporates suggestions made by darin and nisheeth.

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).

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.

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.

Fix checked into the trunk.

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.

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

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???

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.

Added nsbeta1 and topembed due to Bugscape 9169