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 <![CDATA[
> + if (!aListener)
> + throw Components.resources.NS_ERROR_INVALID_ARG;
I don't really see the point of this, and I particularly dislike that the exception is an XPCOM result code...
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
•