Last Comment Bug 666790 - addon download progress doorhanger has error when asking permission to install
: addon download progress doorhanger has error when asking permission to install
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-23 16:24 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2011-06-28 23:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (1.57 KB, patch)
2011-06-23 16:26 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-23 16:24:12 PDT
That summary is less than useful, but take this:

1. Go to https://www.eff.org/https-everywhere
2. Try to install the addon
3. See "Nightly prevented this site from..." dialog
4. Press allow
5. See error in console "Error: this.setProgress is not a function \ Source File: chrome://browser/content/urlbarBindings.xml"

Everything still works. It looks like there are 2 instances of the binding, one created and destroyed before the setTimeout is called. The timeout is never cleared, so it tries to do it's thing on an object that no longer exists. Dave tells me this is because removing a doorhanger in the stack causes the whole thing to be rebuilt so that doesn't sound crazy.
Comment 1 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-23 16:26:21 PDT
Created attachment 541538 [details] [diff] [review]
Patch v0.1

Makes the error go away. I always forget if setting a property for a binding is acceptable this way. But that's what reviews are for :)
Comment 2 Dão Gottwald [:dao] 2011-06-23 16:40:23 PDT
As long as you're aware that the property isn't tied to the binding but directly to the node, it's acceptable. Can you call it _updateProgressTimeout instead of _timeout?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-06-23 21:45:17 PDT
Note that I backed this out of mozilla-inbound because it was one of the two changesets most likely to blame for the Mac64 opt test orange.  Hard to tell exactly, since this landed 2 minutes after the other possibly culprit and hence got coalesced with it....
Comment 4 Dão Gottwald [:dao] 2011-06-23 22:22:45 PDT
Please don't reland this without considering the change from comment 2.
Comment 5 Andrew McCreight [:mccr8] 2011-06-24 09:06:27 PDT
FYI, I tested both this patch and Bug 665564 against m-c after a merge with m-i after our patches landed, and they both came out fine, so I guess it was some kind of build system freakout causing the problem.  Unless our patches interact in some bizarre fashion. ;)

http://tbpl.mozilla.org/?tree=Try&rev=8c56bee019b6
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-28 11:05:26 PDT
Re-landed on inbound with the change Dão suggested.
http://hg.mozilla.org/integration/mozilla-inbound/rev/7313d4e3547f
Comment 7 Joe Drew (not getting mail) 2011-06-28 18:57:12 PDT
http://hg.mozilla.org/mozilla-central/rev/7313d4e3547f

Note You need to log in before you can comment on or make changes to this bug.