407116 - RSS icon in location bar vanishes/disappears near end of page load

Status: VERIFIED FIXED

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

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre

Summary: RSS icon in location bar vanishes/disappears near end of page load

Steps to Reproduce:

If you load either your Gmail inbox or http://netscape.aol.com/, you'll see that the RSS icon in the location bar appears on first load-trigger, but then mysteriously vanishes at some undetermined point in the load (near the end).

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

This regressed between 2007-11-19 (works) and 2007-11-20 (fails):

http://tinyurl.com/2fup58

Comment 2

[Phil Ringnalda (:philor)]

Confirmed by local backout that it's (exposed by) attachment 289203 [details] [diff] [review] from bug 404222. Myk, any idea why those pages are firing rogue misdirected pagehides as part of their loading?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #2)
> Myk, any idea why those pages are firing rogue misdirected pagehides as
> part of their loading?

Hmm, no that's very strange, I'm not sure why this is happening.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Loading netscape.aol.com fires 9-11 pagehide events for http://netscape.aol.com/ads/load.htm . Reloading it fires a page hide event for http://netscape.aol.com/ (as expected), one each for http://netscape.aol.com/ads/load_v4.adp and http://cdn.atwola.com/_media/uac/tcode.html , and another 9-11 for http://netscape.aol.com/ads/load.htm .

Comment 5

[Myk Melez [:myk] [@mykmelez]]

There aren't any iframes in the HTML file, but there are two in the DOM of the loaded document, so I suspect these pagehides are happening as the page reuses an iframe to load multiple pieces of content.

According to http://developer.mozilla.org/en/docs/Using_Firefox_1.5_caching#pagehide_event :

"If you want to define behavior that occurs [when] *the user* navigates away from the page... you can use the new pagehide event."

And also:

"If the page contains frames, then when the cached page is loaded... When *the user* navigates away from the cached page, the pagehide event from each frame fires before the pagehide event in the main document."

(emphases mine)

So it sounds to me like the pagehide event shouldn't be firing for these iframe unloads (if that's what they are), since they don't represent *the user* navigating away from the page, and we should fix this bug.

In the meantime, we have two options to fix the regression:

1. We could revert the patch from bug 404222.
2. We could make browser.xml's pagehide handler check which document is being unloaded before nulling the feeds property.

Of these, option two seems better, as it'll be more obvious that we can remove that check once we fix the rogue pagehide issue.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

I filed the bug on pagehide as bug 407125.

Comment 7

[Phil Ringnalda (:philor)]

You should be able to find some other bugs about disappearing popup notifications to close at the same time, if you let that pageReport nulling in on the check.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes regression

On second thought, we should probably only try to remove the feeds cache once, on pagehide for the topmost page, even once bug 407125 gets fixed, since there will still be legitimate user-spawned pagehides for iframes in the document.

And doing so probably isn't as much of a perf reversion as I thought, since we can compare the pagehide target to the browser's contentDocument property, so we won't have to call getBrowserForDocument, which is what was expensive.

So here's a patch that conditionally clears the cache only if we're pagehiding the topmost document.  With this patch, the feed icon stays around after the load completes on both netscape.aol.com and gmail.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: maybe also fixes notification disappearance bugs

Per comment 7, this patch fixes pageReport nulling so it too only takes place on pagehide for the topmost page.  I'm not familiar with the issues here, so it's not clear that we want to take this patch yet, and thus I'm not obsoleting the first patch.  Phil, how much additional risk does this change entail?

Comment 10

[Phil Ringnalda (:philor)]

In the cold light of morning, "too much risk to just tack onto another bug like this." If you started writing tests for it, like the ones you should be writing for the feeds part, I expect (if my untested guesses are right) that you would find that it would change our current broken behavior of hiding notifications for the top-level window or any subframe on any subframe navigation to new broken behavior of persisting notifications for subframe documents long after they were unloaded.

That's not a problem for feeds, because of our accidental (and I think still untested) behavior of not discovering feeds in subframes, so you don't need to worry about persisting them for too long.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 291852 [details] [diff] [review]
patch v2: maybe also fixes notification disappearance bugs

Canceling review on patch v2 per comment 10, although we should circle around to that at a later date.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: like v1 but removing tabs

I noticed that my earlier patch leaves in place two tabs in the code I'm modifying.  This patch is identical to patch v1 except that it replaces those tabs with spaces.  This is the version of the patch I'll check in.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Checking in toolkit/content/widgets/browser.xml;
/cvsroot/mozilla/toolkit/content/widgets/browser.xml,v  <--  browser.xml
new revision: 1.115; previous revision: 1.114
done

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008010304 Minefield/3.0b3pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010304 Minefield/3.0b3pre

and 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008010304 Minefield/3.0b3pre

on both https://www.gmail.com and http://www.aol.com

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Argh, the fix for this bug contained a typo.  Reopening for the typo fix.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: typo fix followup patch

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Checking in toolkit/content/widgets/browser.xml;
/cvsroot/mozilla/toolkit/content/widgets/browser.xml,v  <--  browser.xml
new revision: 1.117; previous revision: 1.116
done

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED (again :-) ) using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre)
Gecko/2008010904 Minefield/3.0b3pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008010904
Minefield/3.0b3pre

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010904
Minefield/3.0b3pre

Comment 20

[Marcia Knous [:marcia]]

I see the disappearing on aol.com in today's build, Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007121008 Minefield/3.0b2pre. Also seen on nytimes.com and foxnews.com sites.

Comment 21

[Myk Melez [:myk] [@mykmelez]]

I can't reproduce the problem on today's nightlies for Mac or Linux.  Marcia: do you do anything besides loading those three sites (aol.com, nytimes.com, foxnews.com) and waiting for them to finish loading?  How quickly do the icons initially appear when you start loading one of those sites, and how long after that do they disappear?

Comment 22

[Marcia Knous [:marcia]]

I saw it for sure in yesterday's build, but I am not seeing it using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008011105 Minefield/3.0b3pre. I checked the same sites on Mac and not seen there either.

My experience yesterday was that the page would finish loading, and shortly after that the RSS icon would disappear. On the nytimes.com site, if I clicked in the empty content area on the right hand side of the page, the RSS feed would disappear as well. I also checked some other sites, but it did not happen on those sites, but consistently happened on aol.com, nytimes and foxnews.com.

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

Marcia's build ID in https://bugzilla.mozilla.org/show_bug.cgi?id=407116#c20 doesn't contain the fix for this bug, given that it wasn't landed until 2008-01-08; Marcia, can you double-check that you still see this in "yesterday's build", which then would've been 2008-01-09?

Thanks!

Comment 24

[Marcia Knous [:marcia]]

stephend is right on in comment 23, sorry for the noise. resolving back to original status.

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified per comment 19 and comment 24.