597644 - Status of "Subscribe to This Page…" menu and toolbar button is not updated when Back/Forward Navigate

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Alice0775 White]

Build Identifier: 
http://hg.mozilla.org/mozilla-central/rev/6ec3a45f4309
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre ID:20100917195633

After landing of Bug 578967,
Status of "Subscribe to This Page…" menu is not updated when Back/Forward  Navigate between page which deliver RSS and not deliver RSS.


Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Go page that does not deliver the RSS (ex. about:home , https://bugzilla.mozilla.org/ )
3. Go page that delivers RSS(ex. http://planet.mozilla.org/ )
4. You will see that "Subscribe to This Page…" is enabled.
5. Click Back Button and Click Forward Button
6. Check status of "Subscribe to This Page…"

Actual Results:
 "Subscribe to This Page…" is disabled state.

Expected Results:
 State of "Subscribe to This Page…" should be updated.

Comment 1

[Frank Yan (:fryn)]

Does this occur with the Firefox button menu's 'Subscribe' item too?

Comment 2

[Alice0775 White]

(In reply to comment #1)
> Does this occur with the Firefox button menu's 'Subscribe' item too?

Yes.
It seems that regressed before landing Bug 578967 .

So. Please ignore "After landing of Bug 578967," in comment #0.

Comment 3

[Alice0775 White]

"Subscribe to This Page…"in Menu bar is broken in range as follows.
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f4ef40336cc6&tochange=2eb351cc47d3

Comment 4

[Alice0775 White]

I am not sure, but the following modification fix this in my environment.

src/browser/base/content/browser.js 

      // See bug 358202, when tabs are switched during a drag operation,
      // timers don't fire on windows (bug 203573)
      if (aRequest)
-       setTimeout(function () { XULBrowserWindow.asyncUpdateUI(); }, 0);
+       setTimeout(function () { XULBrowserWindow.asyncUpdateUI(); }, 100);
      else
        this.asyncUpdateUI();
    },

Comment 5

[Alice0775 White]

This problem also happens on Linux.
http://hg.mozilla.org/mozilla-central/rev/fc8d8dfee20f
Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100918 Firefox/4.0b7pre ID:20100918030749

And I got same regression range(See comment #3) for Linux build.

Comment 6

[Alice0775 White]

When I revert to changeset 2f5d97dd7e56 in local build, then the problem is gone.
So, I guess that this problem is caused by changeset 350ffc1d793a of bug 512645

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Assigning to vlad since he regressed it! :)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

This wasn't really "caused by" bug 512645 - that bug just made the existing race condition more likely to expose the bug.

We call setTimeout(updateFeed, 0) from the onLocationChange handler, and that's now running before the onLinkAdded that actually populates the "feeds" array on the <browser>.

We should probably just let addFeed (called by the onLinkAdded handler) handle updating the feed, with batching to avoid updating the state many times in rapid succession.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #7)
> Assigning to vlad since he regressed it! :)

I have never touched any of the code here or in any other bugs, for the record; I just filed bug 512645 :-)

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

With this, we could probably remove one of the pageshow/onLocationChange invocations of updateFeeds (via asyncUpdateUI), and probably even remove asyncUpdateUI entirely, but I didn't want to mess with that now.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(that patch is on top of the patch for bug 607496, fwiw)

Comment 14

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/62d05851dd60

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101204 Firefox/4.0b8pre ID:20101204030328

Can we get an automated test for this regression?