347897 - view source on feeds preview shows contents of wrong page

Status: VERIFIED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Darin Fisher]

Using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060808 BonEcho/2.0b1

Steps:
1- navigate to http://news.com.com/2547-1_3-0-20.xml
2- click on a link
3- click back button
4- do view source

Problem:
Source is shown for the link that you clicked on instead of for the feed you are viewing.

Comment 1

[Phil Ringnalda (:philor)]

There must be another step involved somewhere, because with the same build and OS, I get the source for the feed when I go back, no matter which link I follow.

Comment 2

[Darin Fisher]

Weird, I cannot reproduce it either.  I wonder what other step I did to be able to reproduce the problem so easily this morning :-(

Comment 3

[Robert Sayre]

I can see it. OS 10.4 / Trunk 09/13/2006.

Comment 4

[Tom Edwards]

100% reproducible here. Easiest way to repro is hitting back then forward: you get the source of this page.

WinXP

Comment 5

[Robert Sayre]

Yeah, I still have this.

Comment 6

[Courtney Bearse]

I also have this happening.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

1) Visit an feed (checked an rss or atom). ie: http://googleblog.blogspot.com/atom.xml

2) View the source: displays the xml source for the feed.

3) Click on a link in side the feed. 

4) View the source: displays the (x)html source for the page.

5) Click back button.

6) View the source: displays the (x)html source of the page you were just at. It should be displaying the XML source from the feed.

Comment 7

[Asaf Romano (gone)]

Just so we're all on the same page, this is the code in question from viewSource.js:

var PageLoader = getBrowser().webNavigation.QueryInterface(pageLoaderIface);

try {
...
  // Load the page using the page descriptor rather than the URL.
  // This allows the content to be fetched from the cache (if
  // possible) rather than the network...
  PageLoader.loadPage(arg, pageLoaderIface.DISPLAY_AS_SOURCE);
...
}
catch (ex) {
  // Ignore the failure.  The content will be loaded via the URL
  // that was supplied in arg[0].
}

AFAIT LoadPage takes the right document here (which probably isn't cached considering the way feed preview works), but instead of throwing loads the document of the previous SHEntry.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> AFAIT LoadPage takes the right document here 

Really?  I used the following steps:

1)  Start browser
2)  Load http://news.com.com/2547-1_3-0-20.xml
3)  Back
4)  Forward
5)  View source

In LoadPage, the URI of the incoming SHentry is <http://www.mozilla.org/projects/minefield/>.

Checking what GetCurrentDescriptor is returning, it looks like at the point when we call GetCurrentDescriptor the history entry of the docshell is the entry for the document in bfcache.

So what the _heck_ is RSS doing to cause this situation?  ;)  This should _so_ not be happening.  Like "code not designed to handle this" not happening.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  So when loading the RSS feed from history I first hit nsDocShell::EndPageLoad, called when the HTTP channel is removed from the loadgroup.  _Then_ I hit nsDocShell::Embed when the jar channel calls OnStartRequest.

That's backwards.  So we first null out mLSHE in EndPageLoad, then have nothing to set mOSHE to in Embed.

Comment 10

[Asaf Romano (gone)]

I didn't actually inpect the SHEntry, just the incoming content document...

Comment 11

[Boris Zbarsky [:bzbarsky]]

  handleResult: function FC_handleResult(result) {

(in FeedConverter.js) never sets a loadgroup on the channel it opens.  That's the problem.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Back to where this bug came from.  This is so not a core Gecko issue.  ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Just to clarify my nomination, this code is putting the docshell in an inconsistent and unexpected state.  I can't guarantee that it doesn't crash, isn't exploitable, etc.

Comment 14

[Robert Sayre]

Attachment: set the loadGroup

Comment 15

[Asaf Romano (gone)]

Comment on attachment 245976 [details] [diff] [review]
set the loadGroup

interactive r=mano.

Comment 16

[Robert Sayre]

Checking in FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.22; previous revision: 1.21
done

Comment 17

[Boris Zbarsky [:bzbarsky]]

Land this on branch too?

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 245976 [details] [diff] [review]
set the loadGroup

approved for 1.8 branch, a=dveditz for drivers

Comment 19

[Robert Sayre]

Checking in FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.1.2.21; previous revision: 1.1.2.20
done

Comment 20

[Jay Patel [:jay]]

v.fixed on 1.8.1 branch and Trunk/1.9 with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20061128 Minefield/3.0a1

View->Page Source shows the proper source for feed xml page.