Wrong pane is being displayed in "About Nightly": (Nightly is being updated by another instance)

RESOLVED FIXED in Firefox 34

Status

()

Core
Widget: Cocoa
--
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aokoye, Assigned: ttaubert)

Tracking

({dev-doc-needed, regression})

35 Branch
mozilla35
All
Mac OS X
dev-doc-needed, regression
Points:
---

Firefox Tracking Flags

(firefox32 wontfix, firefox33 wontfix, firefox34+ fixed, firefox35+ fixed, firefox-esr31 wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8490444 [details]
Screen Shot 2014-09-16 at 4.10.10 PM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20140916004001

Steps to reproduce:

1. Go to Nightly -> About Nightly
2. Click the prompt to update nightly to the most current version
3. Wait for it to download and click the prompt to restart the browser
4. When the browser has restarted do step number 1 again to check if it updated successfully - it will say that "Nightly is being updated by another instance"


Actual results:

When I did step 4 (aka step 1: Go to Nightly -> About Nightly) it said "Nightly is being updated by another instance" instead of saying that it had successfully or unsuccessfully updated.

Note: it appears (from asking two people to repeat this) that nightly was already at the latest update that is currently available from nightly.mozilla.org - 35.0a1 (as of 9/16/14, 16:17PST).


Expected results:

The window should have indicated that the build update had successfully or unsuccessfully updated.
I'm also seeing this issue.

What's surprising, is that if I'm reading the code right, the message "Nightly is being updated by another instance" should never be capable of being displayed on Mac...

I put a breakpoint (via browser toolbox) in UpdateService#isOtherInstanceHandlingUpdates and that correctly returns false.

However, I did notice that after coming out of the breakpoint, the dialog then correctly said "Nightly is up to date". Doing the request again without the breakpoint then puts it back into the "another instance" state.
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: Untriaged → Application Update
Ever confirmed: true
Keywords: regression
Product: Firefox → Toolkit
Hardware: x86 → All
Summary: Nightly won't update via the "About Nightly" window → Nightly won't update via the "About Nightly" window (Nightly is being updated by another instance)
(Assignee)

Comment 2

3 years ago
This is weird. I added some debugging and this is what's happening:

 ++ isOtherInstanceHandlingUpdates = false
 ++ checkForUpdates()
 ++ selectPanel <image xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" class="update-throbber"/><label xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">Checking for updates…</label>
 ++ onCheckComplete()
 ++ noUpdatesFound
 ++ selectPanel <label xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">Nightly is up to date</label>

It is selecting the right panel after checking for updates but I still end up with the message about another instance.
(Assignee)

Comment 3

3 years ago
this.updateDeck.selectedPanel.innerHTML returns the right content as well, although a different panel is displayed.
(Assignee)

Comment 4

3 years ago
Git bisect points me to bug 1044595... Any idea Markus?
Blocks: 1044595
This seems like a widget or front end issue, I'm going to move out of app update for now. But if we find that's not the case feel free to move back over to app update.
Component: Application Update → Widget: Cocoa
Product: Toolkit → Core
Summary: Nightly won't update via the "About Nightly" window (Nightly is being updated by another instance) → Wrong pane is being displayed in "About Nightly": (Nightly is being updated by another instance)
(In reply to Tim Taubert [:ttaubert] from comment #4)
> Git bisect points me to bug 1044595... Any idea Markus?

Huh, are you sure about that?

If I change the value of the selectedIndex attribute of the deck using DOM Inspector, it looks like the displayed panel is always off by one; e.g. selectedIndex="0" displays the second hbox child of the deck ("#downloadAndInstall") instead of the first one.
(Assignee)

Comment 7

3 years ago
Yeah, reverting that cset fixes it. Commenting out some of the CSS fixes it too:

button[default="true"]:not(:hover):active {
  color: ButtonText;
}

That seems to be what's causing it.
Amazing.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
(Assignee)

Comment 9

3 years ago
Hmmm :/ Sorry, it seems that this is maybe intermittent? It's certainly not fixed with the cset reverted now locally. *sigh*
(Assignee)

Comment 10

3 years ago
Definitely not the bug I blamed, seems to be a tad older. Sorry Markus!
No problem. I'd guess bug 1009628 based on the blame of nsDeckFrame.cpp, but that looks like it's already too old...
(Assignee)

Updated

3 years ago
No longer blocks: 1044595
(Assignee)

Comment 12

3 years ago
The problem seems to be that the first panel of the deck is set to hidden=true. That causes the frame to removed and nsDeckFrame forgets about it. The XUL binding however still uses it to determine the index when .selectedPanel is set. We end up with the index being 9 which is now the "other instance" message.
Assignee: mstange → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 13

3 years ago
We should fix that disconnect between how the <deck> counts its frames and how the XUL binding counts its panels. Looking at the current code it seems that we wouldn't handle inserting frames correctly either, i.e. when the first panel would be unhidden again.
(Assignee)

Comment 14

3 years ago
Fun. From the <deck> documentation on MDN:

"Do not hide panels; the deck element only understands visible panels."
(In reply to Tim Taubert [:ttaubert] from comment #13)
> We should fix that disconnect between how the <deck> counts its frames and
> how the XUL binding counts its panels. Looking at the current code it seems
> that we wouldn't handle inserting frames correctly either, i.e. when the
> first panel would be unhidden again.

Oh man. Yeah. I remember running into this before. Logs:

http://logs.glob.uno/?c=mozilla%23fx-team&s=5+Dec+2013&e=5+Dec+2013

Looks like I found:

https://bugzilla.mozilla.org/show_bug.cgi?id=390724#c1

and considered it filed.

I also vaguely recall saying that we should fix selectedIndex to work against real children instead of just visible ones, but that someone (Neil?) had some objections... but maybe I'm wrong - certainly can't find the logs! It certainly seems like the most sensible solution from what's going on here...
Flags: needinfo?(neil)
(Assignee)

Comment 16

3 years ago
It is tempting to just not hide the first panel in the about dialog... but I would lean towards actually fixing this. Seems like a real foot gun if not even we can get it right.
What's the use case for setting the selected index to a hidden panel? If there isn't, maybe we should just force all deck children to be visible?
Flags: needinfo?(neil)
(Assignee)

Comment 18

3 years ago
We're not setting the index to a hidden panel. What happens here is that the very first panel of the <deck> gets hidden and then setting .selectedPanel fails in that it shows the next sibling instead of the desired one.
(Assignee)

Comment 19

3 years ago
Created attachment 8491407 [details] [diff] [review]
0001-Bug-1068384-Make-xul-deck-support-hidden-panels.patch

How about something like this? WFM locally. We would simply ignore if someone sets .selectedPanel to a hidden panel and would support any number of hidden panels in a <deck> while still maintaining a correct index as seen from the XUL/JS side of things. We also wouldn't need to watch frame additions or removals anymore.

If you think that's a valid solution we should of course add a few tests to make sure everything works as expected.
Attachment #8491407 - Flags: review?(neil)
(Assignee)

Comment 20

3 years ago
Looks like I was wrong about not having to watch additions or removals. I didn't think about actually removing or adding nodes. We would of course need to adapt the index when that happens.
Comment on attachment 8491407 [details] [diff] [review]
0001-Bug-1068384-Make-xul-deck-support-hidden-panels.patch

>+  nsIFrame* box = nsBox::GetChildBox(this);
>+
>+  // Run through each child frame to find the selected one.
>+  while (box)
>+  {
>+    if (selectedContent == box->GetContent()) {
>+      return box;
>+    }
>+
>+    box = GetNextBox(box);
>+  }
>+
>+  return nullptr;
(This is almost the same as GetChildBoxForContent which is static to nsSplitterFrame.cpp, I wonder whether it's worth a followup to move that to a static method on nsBox which you can call from here?)

> void
>-nsDeckFrame::RemoveFrame(ChildListID aListID,
>-                         nsIFrame* aOldFrame)
This was for bug 1009628 so you might want to avoid regressing that.

>+  nsIFrame* selectedBox = GetSelectedBox();
Rather than searching for the selected box and then checking each box, it might be easier to check each box to see if its content is the selected content.

>+  while (box)
>   {
>-    // make collapsed children not show up
>-    if (count != mIndex) 
>+    // Hide current box if not the selected one.
>+    if (!selectedBox || selectedBox != box) {
Don't need to null-check selectedBox; box is known not to be null here, so !selectedBox implies selectedBox != box already.
Attachment #8491407 - Flags: review?(neil) → feedback+
Possibly related, with 2014-09-16 Nightly on OS X (and also with 2014-09-18 nightly), if I set Firefox to "Never check for updates" (options -> advanced -> updates), and then open the about dialog, instead of seeing a button "Check for updates" I see a button with no text, and clicking it (few times, with some time between) doesn't seem to trigger update, and effectively doesn't let me update the build (for 2014-09-18 there's no newer build right now, but on 2014-09-16 there was and it still didn't let me update).

Clicking the (empty) button at the about dialog shows the following at the error console:

> TypeError: this.update is undefined on chrome://brower/content/aboutDialog.js:261
(Assignee)

Comment 23

3 years ago
So nsMenuBarX::AquifyMenuBar() hides the element with ID "checkForUpdates" which happens to be the first panel in the about dialog's <deck>. The comment is talking about a menu item, so is that hiding the right element here? Why did this work before? I can't find any other elements with the same ID...
Flags: needinfo?(mstange)
Hah, nice! It used to be a menu item, but it was removed here: http://hg.mozilla.org/mozilla-central/rev/726e585dd529
Flags: needinfo?(mstange)
(Assignee)

Comment 25

3 years ago
Created attachment 8492120 [details] [diff] [review]
0001-Bug-1068384-Remove-legacy-code-that-handled-the-app-.patch

Removing all legacy code handling the update menu item sounds like the right thing to do here.
Attachment #8492120 - Flags: review?(mstange)
Comment on attachment 8492120 [details] [diff] [review]
0001-Bug-1068384-Remove-legacy-code-that-handled-the-app-.patch

Review of attachment 8492120 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like SeaMonkey still uses this functionality: http://mxr.mozilla.org/comm-central/source/suite/common/utilityOverlay.xul#475

Can we instead rename the ID to something that is less likely to collide? e.g. menu_checkForUpdates
Attachment #8492120 - Flags: feedback?(stefanh)
(Assignee)

Comment 27

3 years ago
Sigh. Another option would be for SeaMonkey to get rid of the menu item as well.
Or just live with it being in the help menu. Stefan, how important is having the item in the app menu for you?

Comment 29

3 years ago
Thanks for asking Markus. I'll see what options we have when I get home from work.

Comment 30

3 years ago
Neil, can we get rid of the menuitem and follow thunderbird/firefox here?
Flags: needinfo?(neil)
(In reply to Stefan from comment #30)
> Neil, can we get rid of the menuitem and follow thunderbird/firefox here?

Right now, we don't even have an About dialog.
Flags: needinfo?(neil)
Duplicate of this bug: 1070201
Comment on attachment 8491407 [details] [diff] [review]
0001-Bug-1068384-Make-xul-deck-support-hidden-panels.patch

This patch is going to regress bug 1009628.

STR:

1) Open a new e10s window
2) Open a new tab and select it (you should now have two tabs - with the second one selected)
3) Browse to some location in the second tab
4) Without selecting it, close the first tab.

ER:

We should continue to see the second tab's content

AR:

We see no content. :(

The changes to XUL deck were to account for this case - we used to sidestep this by immediately switching to the selectedPanel after removing a tab, but that doesn't work in our new asynchronous tab world... we see a flash of white or black before rendering with that technique.
Attachment #8491407 - Flags: feedback-
Also, the patch for bug 1009628 landed on the 34 train, which is currently on Aurora... do we know of people experiencing this problem on Aurora?
(Assignee)

Comment 35

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #33)
> This patch is going to regress bug 1009628.

Yeah, I realized that too after posting it. I don't think we will pursue this way any further. See comment #23 for what's happening here.
(Assignee)

Updated

3 years ago
Attachment #8491407 - Attachment is obsolete: true

Comment 36

3 years ago
Comment on attachment 8492120 [details] [diff] [review]
0001-Bug-1068384-Remove-legacy-code-that-handled-the-app-.patch

I think I can live with the menuitem being in the help menu for a cycle or two (I would like SeaMonkey to have an About dialog with the update functionality).

That said, there's more to remove in nsMenuBarX.mm:
- Line 603 (the comment)
- Line 647-656

Also: this menuitem is documented at devmo, so it should be flagged for removal from there.
Attachment #8492120 - Flags: feedback?(stefanh) → feedback+

Comment 37

3 years ago
(In reply to Stefan [:stefanh] from comment #36)
> Also: this menuitem is documented at devmo, so it should be flagged for
> removal from there.

Sorry, forgot to link to the doc I found: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/PlatformMenus
Comment on attachment 8492120 [details] [diff] [review]
0001-Bug-1068384-Remove-legacy-code-that-handled-the-app-.patch

r=me with Stefan's comments addressed
Attachment #8492120 - Flags: review?(mstange) → review+
I've started getting this error on OSX. The problem started less than a week ago. There is a similar issue filed, bug 958554.

http://i.imgur.com/HXf3gJs.png
(Assignee)

Comment 40

3 years ago
(In reply to Dane MacMillan from comment #39)
> I've started getting this error on OSX. The problem started less than a week
> ago. There is a similar issue filed, bug 958554.

The bug you referenced here is actually Windows-specific. This message should never be shown on OS X at all, so this bug here is OS X specific.
(Assignee)

Comment 41

3 years ago
[Tracking Requested - why for this release]:

This breaks manual updates on OS X and displays a very misleading message when auto updates are enabled.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
(Assignee)

Comment 42

3 years ago
The error was reported for Firefox 34, although I can only reproduce it on 35. I don't see a reason why this would suddenly start to fail. Considering when the menu item was removed we should have had this failure for a while now...
(Assignee)

Comment 43

3 years ago
(In reply to Stefan [:stefanh] from comment #37)
> (In reply to Stefan [:stefanh] from comment #36)
> > Also: this menuitem is documented at devmo, so it should be flagged for
> > removal from there.
> 
> Sorry, forgot to link to the doc I found:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/
> PlatformMenus

Not sure what to update this to. Should this say it's now Seamonkey-only and has been removed from Firefox in 2010 already? Would be great if someone else could do this that understands the help and app menu structure a little better.
Keywords: dev-doc-needed
(Assignee)

Comment 44

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/e42e0e8f3791

Comment 45

3 years ago
(In reply to Tim Taubert [:ttaubert] from comment #43)
> Not sure what to update this to. Should this say it's now Seamonkey-only and
> has been removed from Firefox in 2010 already? Would be great if someone
> else could do this that understands the help and app menu structure a little
> better.

dev-doc-needed was what I thought about when I said flagged for removal :-) I think the checkForUpdates item can just be removed once Firefox 34 is out.

Comment 46

3 years ago
(In reply to Stefan [:stefanh] from comment #45)
> dev-doc-needed was what I thought about when I said flagged for removal :-)
> I think the checkForUpdates item can just be removed once Firefox 34 is out.

If the patch lands on Aurora, of course.
(In reply to Tim Taubert [:ttaubert] from comment #42)
> The error was reported for Firefox 34, although I can only reproduce it on
> 35. I don't see a reason why this would suddenly start to fail. Considering
> when the menu item was removed we should have had this failure for a while
> now...

I don't think we'd take this fix on ESR but can consider it for 33 if that version is impacted as well. Do you know how far back this issue goes?
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox34: ? → +
tracking-firefox35: ? → +
Flags: needinfo?(ttaubert)
status-firefox32: --- → ?
status-firefox33: --- → ?
tracking-firefox33: --- → ?
(Assignee)

Comment 48

3 years ago
(In reply to Lawrence Mandel [:lmandel] from comment #47)
> Do you know how far back this issue goes?

Unfortunately not. I can reproduce it on 35 and this report was opened for 34. Can't reproduce it on 33. As long as we don't see any reports for 33 I'd say we don't track this?
Flags: needinfo?(ttaubert)
Agreed. I'm clearing status for 32 and 33 instead of marking unaffected as we don't know for certain that that is the case.
status-firefox32: ? → ---
status-firefox33: ? → ---
tracking-firefox33: ? → ---

Comment 50

3 years ago
So looking around and I can't tell if my bug is related to this: https://bugzilla.mozilla.org/show_bug.cgi?id=1070473
https://hg.mozilla.org/mozilla-central/rev/e42e0e8f3791
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox35: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Comment 52

3 years ago
(In reply to Jonny Barnes from comment #50)
> So looking around and I can't tell if my bug is related to this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1070473

Not related. That seems to be another issue, possibly with restart being broken on Yosemite.
Tim - This bug is marked as 34 affected. Can you please submit an Aurora approval request?
Flags: needinfo?(ttaubert)
(Assignee)

Comment 54

3 years ago
Comment on attachment 8492120 [details] [diff] [review]
0001-Bug-1068384-Remove-legacy-code-that-handled-the-app-.patch

Approval Request Comment
[Feature/regressing bug #]: bug 599480
[User impact if declined]: OS X users may see "Nightly is being updated by another instance" in the About dialog even though the browser is up-to-date.
[Describe test coverage new/current, TBPL]: No tests, verified manually and no reports since then.
[Risks and why]: Low risk, the patch removes code that was handling a menu item that was removed in 2010.
[String/UUID change made/needed]: None.
Attachment #8492120 - Flags: approval-mozilla-aurora?
Flags: needinfo?(ttaubert)
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox-esr31: --- → wontfix
Comment on attachment 8492120 [details] [diff] [review]
0001-Bug-1068384-Remove-legacy-code-that-handled-the-app-.patch

Aurora+
Attachment #8492120 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 56

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/02e316668804
status-firefox34: affected → fixed
You need to log in before you can comment on or make changes to this bug.