Open Bug 320663 Opened 19 years ago Updated 2 years ago

addProgressListener from Tab Browser does not always honor filter flags

Categories

(Toolkit :: UI Widgets, defect)

defect

Tracking

()

UNCONFIRMED

People

(Reporter: mark, Unassigned)

Details

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Add a Progress Listener with filter flags like this:
    gBrowser.addProgressListener(browserProgressListener, 
	Components.interfaces.nsIWebProgress.NOTIFY_STATE_ALL | 
	Components.interfaces.nsIWebProgress.NOTIFY_LOCATION);

Then in a browser with multiple tabs, click on a new tab. The onSecurityChange handler is called (incorrectly). (If you do not have an onSecurityChange handler defined, the dispatcher will fail silently, and all other progress events are lost.)

The calls appear to be in toolkit/content/widgets/tabbrowser.xml, e.g:

682             for (i = 0; i < this.mProgressListeners.length; i++) {
683               p = this.mProgressListeners[i];
684               if (p) {
685                 p.onLocationChange(webProgress, null, loc);
686                 if (securityUI)
687                   p.onSecurityChange(webProgress, null, securityUI.state);

The filter flags are ignored.

[Also, shouldn't that loop work "backwards" in case listeners are removed by the handlers?]

The user-visible manifestation of this was that when my extension (which had no dummy handler for onSecurityChange) was installed, the browser title bar failed to update and fastFind worked only in the first tab. This is because those are set up after line 687 above.


Reproducible: Always



Expected Results:  
Making the filters work according to spececification will probably break existing extensions. 

Fixing the event dispatch loop to check if the handler exists would avoid some difficult-to-find bugs. This was done for onLinkIconAvailable.

At a minimum, this should be documented.
also, that function should probably use try..catch so that exceptions from one handler don't affect others.
Well, you're probably supposed to implement nsIWebProgressListener fully. onLinkIconAvailable is not a part of the interface, so that's why it's checked for.

(That is not to say that the current code is all correct and clear.)
Component: General → XUL Widgets
Product: Firefox → Toolkit
QA Contact: general → xul.widgets
Version: unspecified → Trunk
As a workaround, you can just implement unused handlers as follows:

onSecurityChange: function () {
    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
}

This shouldn't harm program flow but continues to remind you of the unfixed bug.
(In reply to comment #3)

> onSecurityChange: function () {
>     throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> }

Although this would be the best solution, it sometimes seems to lead to problems with an uncaught internal exception being returned at some point. A less risky way is to simply silently back out using something along the lines of:

onSecurityChange: function () {
    return;
}

HTH.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.