396847 - Feed (RSS) and OpenSearch indicators can disappear due to subresource loads

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Doug Turner (:dougt)]

1) load cnn.com
2) notice the rss icon
3) press command h to hide the app
4) bring camino to the foreground
5) notice that the rss icon is missing.

Seams to work fine on other sites with rss.

Comment 1

[Samuel Sidler (old account; do not CC)]

It happens on its own... you don't need to defocus the app, just wait.

Comment 2

[John P Baker]

This looks similar to bug 407116

Comment 3

[Samuel Sidler (old account; do not CC)]

That's a Firefox bug and a recent (November) regression. This is a Camino bug and is older than that.

Comment 4

[Nick Kreeger]

Hrmm... I've been trying to reproduce this for a little while now on 1.5.3

Comment 5

[Nep]

Here's an additional case, happening in 1.6:

1) load http://vancouverethniceats.wordpress.com/
2) note that the RSS icon disappears
3) Remove this snap.com javascript code from the page: <script defer="defer" id="snap_preview_anywhere" type="text/javascript" src="http://spa.snap.com/snap_preview_anywhere.js?ap=0&amp;si=1&amp;sb=1&amp;key=e5a3c1e6dff5de35b1c6e7c470411886&amp;domain=vancouverethniceats.wordpress.com&amp;fl=wordpress&amp;pub=pub-2311827-www.wordpress.com&amp;es=all&amp;lang=en"></script>
4) note that the icon stays

TTFN
Travis

Comment 6

[Chris Lawson (gone)]

We need a reduced testcase for this. Thanks for finding another example, Travis -- that may help in working out a reduced testcase.

With a trunk build, it seems that mousing over any of the Flickr photos is what causes the RSS icon to (immediately) disappear at the URL in comment 5. I'm looking into the script to see if I can figure out how this might be happening.

Comment 7

[Chris Lawson (gone)]

Attachment: minimal HTML testcase

Here's a fairly minimal HTML testcase that contains just enough HTML to provide an RSS <link> element and call the script that's causing this to happen.

Still working on figuring out how the script is creating the problem, though.

Comment 8

[Chris Lawson (gone)]

A guess at what's happening: the external script is (somehow) causing another onLoadingStarted to happen

http://mxr.mozilla.org/mozilla/source/camino/src/browser/BrowserWrapper.mm#507

which is clearing out any detected feeds.

The most likely source of this onLoadingStarted call seems to be

http://mxr.mozilla.org/mozilla/source/camino/src/embedding/CHBrowserListener.mm#686

since it seems that the script is generating some network activity.

Comment 9

[Chris Lawson (gone)]

Nick, the way Myk fixed bug 407116 may be helpful to you here.

I'm not sure if we have access, via the embedding code, to the same hooks that Firefox does, but upon further inspection, it looks like that bug was being caused by a very similar problem. (Thanks, John!)

Comment 10

[Chris Lawson (gone)]

Attachment: fix

Here's a fix that Nick suggested tonight on IRC and that, on further inspection, I believe to be the right fix. It fixes the testcase and jalopnik.com (where I also noticed the problem tonight).

Comment 11

[Chris Lawson (gone)]

Murph, would you mind taking a look at this too? I moved the search detection stuff as well, because it looks to me like it would have been vulnerable to the same issue as the feed stuff was.

Comment 12

[Nick Kreeger]

Comment on attachment 319907 [details] [diff] [review]
fix

r=me

Does |newPage| mean that the content about to be loaded is from a different web page? If so, we should move the the stuff into the |if (newPage) {| block.

Comment 13

[Chris Lawson (gone)]

Attachment: fix v1.1

(In reply to comment #12)
> (From update of attachment 319907 [details] [diff] [review])
> r=me
> 
> Does |newPage| mean that the content about to be loaded is from a different web
> page? If so, we should move the the stuff into the |if (newPage) {| block.

Yeah, upon further digging, it appears to. Here's a new patch that also does a bunch of whitespace cleanup (whitespace on blank lines all over that file) while we're touching the file.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #11)
> Murph, would you mind taking a look at this too? I moved the search detection
> stuff as well, because it looks to me like it would have been vulnerable to the
> same issue as the feed stuff was.

It is; see bug 420795.  Chris, is this patch ready for sr?

Comment 15

[Chris Lawson (gone)]

Comment on attachment 319911 [details] [diff] [review]
fix v1.1

Yeah, this is ready for sr, with or without murph's review, although I'd still like him to take a look at it.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Tab titles and site icons have this same problem; I filed bug 434266 on those since this patch is just done cooking.

Comment 18

[Sean Murphy]

Comment on attachment 319911 [details] [diff] [review]
fix v1.1

Sorry for taking a while to look at this cl, r=murph.

Comment 19

[Stuart Morgan]

Comment on attachment 319911 [details] [diff] [review]
fix v1.1

sr=smorgan

In the future, when you want to do widespread style cleanup, do it in a different patch rather than coupling it to a tiny change; all the unrelated churn makes the patch much harder to review.

Comment 20

[Chris Lawson (gone)]

Attachment: fix v1.2, no whitespace changes

Here's the fix without the whitespace changes.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'll land this in two parts, code and whitespace separately.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, Chris and I had a communication problem :( I had planned to just land this patch in two commits to make the blame situation clear, but I guess Chris thought I wanted him to split it into two bugs entirely when I asked if he could split the patch up for me, since he filed bug 437561 :(

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

"fix v1.2" checked in on the trunk.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

"fix v1.2" checked in on the MOZILLA_1_8_BRANCH in advance of 1.6.2.