Open
Bug 560512
Opened 15 years ago
Updated 2 years ago
Check argument to addProgressListener
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
NEW
People
(Reporter: john.p.baker, Unassigned)
References
Details
This is part of Seamonkey bug 255503 that isn't addressed in bug 519216.
We check |if (aMethod in p)| or, currently, various sequences like |if ('onLinkIconAvailable' in p)|
but we don't check the listener on addProgressListener. At the very least we should copy
Seamonkey and do
1665 <method name="addProgressListener">
1666 <parameter name="aListener"/>
1667 <parameter name="aMask"/>
1668 <body>
1669 <![CDATA[
+ if (!aListener)
+ throw Components.resources.NS_ERROR_INVALID_ARG;
+
1670 if (!this.mAddProgressListenerWasCalled) {
1671 this.mAddProgressListenerWasCalled = true;
1672 this.tabContainer.updateVisibility();
1673 }
[Possibly also a check for (aListener instanceOf Object) but I am not sure if that would break something]
This is particularly important if the patch in bug 519216 is applied as some instances will no longer be caught; For instance
if (p)
try {
p.onLocationChange(...
will, in effect, become
if ('onLocationChange in p) {
try {
p['onLocationChange'].apply(p, ...
As usual discussion applies to addTabsProgressListener as well.
|
||
Comment 1•15 years ago
|
||
(In reply to comment #0)
> At the very least we should copy Seamonkey and do
>
> 1665 <method name="addProgressListener">
> 1666 <parameter name="aListener"/>
> 1667 <parameter name="aMask"/>
> 1668 <body>
> 1669 <
|
Reporter | |
Comment 3•15 years ago
|
||
At the moment, if an extension does gBrowser.addProgressEvent(listener) where listener happens to be null or undefined [or not an Object] this will be added to mProgressListeners but be protected by the if (p) [or often the try block] that is used before calling the listener functions.
With the patch in bug 519216 these will still be stored in mProgressListeners but now |if (aMethod in p)| will throw "invalid 'in' operand p" and prevent any further listeners or UI updates from happening.
A different solution would be to change the patch in bug 519216 to do
+ try {
+ if (aMethod in p) {
+ if (!p[aMethod].apply(p, aArguments))
+ rv = false;
+ }
+ } catch (e) {
+ // don't inhibit other listeners
+ Components.utils.reportError(e);
+ }
[which might report a lot]
|
|
||
Comment 4•15 years ago
|
||
As long as we report some error at some point, I think we're good. There are numerous places where improper arguments could be passed, but we cannot cover all of them (or rather, I don't think we'd want to, as it would be massive bloat). And addProgressListener doesn't seem to be an exceptional case that would more likely than others get improper arguments.
|
||
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•