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

RESOLVED FIXED in Firefox 19

Status

()

defect
--
trivial
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: xabolcs, Assigned: xabolcs)

Tracking

Trunk
Firefox 19
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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/
Assignee: nobody → xabolcs
Posted patch Changing onUnload (obsolete) — Splinter Review
Modify onUnload, targeting performance
Attachment #654552 - Flags: review?(robert.bugzilla)
Posted patch Changing appUpdater.prototype (obsolete) — Splinter Review
Modify appUpdater.prototype, targeting safety
Attachment #654553 - Flags: review?(robert.bugzilla)
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
Status: NEW → ASSIGNED
Ping.
Attachment #654552 - Flags: review?(robert.bugzilla) → review?(netzen)
Attachment #654553 - Flags: review?(robert.bugzilla) → review?(netzen)
(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 (...) {
}
Attachment #654552 - Flags: review?(netzen)
Attachment #654553 - Flags: review?(netzen)
Posted patch Changing onUnload - v2 (obsolete) — Splinter Review
Attachment #654552 - Attachment is obsolete: true
Attachment #674780 - Attachment is patch: true
Attachment #674780 - Attachment mime type: application/unknown → text/plain
Attachment #674780 - Flags: review?(netzen)
Attachment #674781 - Flags: review?(netzen)
Attachment #654553 - Attachment is obsolete: true
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");
Attachment #674781 - Flags: review?(netzen) → review+
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!
Attachment #674780 - Flags: review?(netzen) → review-
(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 on attachment 674780 [details] [diff] [review]
Changing onUnload - v2

Decided to use the other approach.
Attachment #674780 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8a58f6111c99
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.