785015 - Closing About dialog when there is already an update window open causes onUnload() to fail (Error: TypeError: this.aus is undefined)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Szabolcs Hubai (:xabolcs)]

STR:
0. Set logging preferences if needed
1. Open Update wizard somehow (e.g. with an addon[1])
2. Open About dialog
2.1 Check that there is no (update/restart/...) button
3. Close About Dialog
4. Check Error Console for errors
5. Error: TypeError: this.aus is undefined logged into console



The problem is that onUnload() unsafely calls appUpdater.removeDownloadListener() even when gAppUpdater.aus wasn't
set up (because of Update Wizard was open).


[1] - https://addons.mozilla.org/addon/help-menu-update/

Comment 1

[Szabolcs Hubai (:xabolcs)]

Attachment: Changing onUnload

Modify onUnload, targeting performance

Comment 2

[Szabolcs Hubai (:xabolcs)]

Attachment: Changing appUpdater.prototype

Modify appUpdater.prototype, targeting safety

Comment 3

[Szabolcs Hubai (:xabolcs)]

About the patch attachments: I just hg inited an aboutDialog.js grabbed from hg.m.o [1].

Should I provide these patches based on a proper checkout?


[1] - https://hg.mozilla.org/mozilla-central/raw-file/5650196a8c7d/browser/base/content/aboutDialog.js

Comment 4

[Szabolcs Hubai (:xabolcs)]

Ping.

Comment 5

[Brian R. Bondy [:bbondy]]

(In reply to Szabolcs Hubai (:xabolcs) from comment #3)
> About the patch attachments: I just hg inited an aboutDialog.js grabbed from
> hg.m.o [1].
> 
> Should I provide these patches based on a proper checkout?
> 
> 
> [1] -
> https://hg.mozilla.org/mozilla-central/raw-file/5650196a8c7d/browser/base/
> content/aboutDialog.js

Yes please, they fail to apply.  Otherwise the patches look good.

nit: Please use this format:

if (...) {
}

Comment 6

[Szabolcs Hubai (:xabolcs)]

Attachment: Changing onUnload - v2

Comment 7

[Szabolcs Hubai (:xabolcs)]

Attachment: Changing appUpdater.prototype - v2

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 674781 [details] [diff] [review]
Changing appUpdater.prototype - v2

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

Looks good.
You called this out in Comment 0, but for reference this is why the problem happens.

  // Hide the update deck when there is already an update window open to avoid
  // syncing issues between them.
  if (Services.wm.getMostRecentWindow("Update:Wizard")) {
    this.updateDeck.hidden = true;
    return; <-----------------------
  }

  XPCOMUtils.defineLazyServiceGetter(this, "aus",
                                     "@mozilla.org/updates/update-service;1",
                                     "nsIApplicationUpdateService");

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 674780 [details] [diff] [review]
Changing onUnload - v2

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

I don't think this is needed, please let me know if you disagree. And if so, why you think it is needed.

Thanks for the patches!

Comment 10

[Szabolcs Hubai (:xabolcs)]

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> Comment on attachment 674780 [details] [diff] [review]
> Changing onUnload - v2
> 
> Review of attachment 674780 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this is needed, please let me know if you disagree. And if so,
> why you think it is needed.
> 
> Thanks for the patches!

Thanks for the quick response!

Sorry, I didn't write but these patches 
wanted to exclude each other - as You guessed. :)
So yes, applying only one patch is more than enough!



(In reply to Brian R. Bondy [:bbondy] from comment #8)
> ...
> You called this out in Comment 0, but for reference this is why the problem
> happens.
> 
>   // Hide the update deck when there is already an update window open to
> avoid
>   // syncing issues between them.
>   if (Services.wm.getMostRecentWindow("Update:Wizard")) {
>     this.updateDeck.hidden = true;
>     return; <-----------------------
>   }
> 
>   XPCOMUtils.defineLazyServiceGetter(this, "aus",
>                                      "@mozilla.org/updates/update-service;1",
>                                      "nsIApplicationUpdateService");

Yes, the problem was the early return, which happened too early sometimes. ;)

Comment 11

[Szabolcs Hubai (:xabolcs)]

Comment on attachment 674780 [details] [diff] [review]
Changing onUnload - v2

Decided to use the other approach.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a58f6111c99

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a58f6111c99