608628 - aMask argument to tabbrowser's addProgressListener is only used when not in tabbed mode

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

In the common case (not in tabbed mode), the aMask parameter is ignored. We should either get rid of it, or re-implement the re-dispatching such that it works.

Comment 1

[Dão Gottwald [:dao]]

Note that tabbed mode isn't just the common case but pretty much the only case in practice, since we call addTabsProgressListener in browser.js. Any second addProgressListener call would trigger tabbed mode, too (the first one being the one in browser.js).

Add-ons use NOTIFY_STATE_* quite often, but it's hard to tell whether starting to honor it would break them or fix subtle bugs.

mozilla-central has two cases, NOTIFY_ALL in browser.js and NOTIFY_STATE_DOCUMENT in browser_keywordSearch.js. The former is a no-op and the latter fails because it's accessed on the wrong interface.

NOTIFY_PROGRESS, NOTIFY_STATUS, NOTIFY_SECURITY, NOTIFY_LOCATION and NOTIFY_REFRESH and NOTIFY_ALL are redundant since the callbacks are optional.

Last but not least, adding support for the masks probably isn't trivial (e.g. they need to be stored somewhere).

So I think we should remove the parameter.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> the latter fails because it's accessed on the wrong interface.

Yeah, that's how I found this - I'm fixing it in bug 608198.

Removing it sounds good to me.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 487308 [details] [diff] [review]
patch

The two test_tabbrowser.xuls have calls to tabBrowser.addProgressListener() that need updating too.

I don't think we really want to take this before branching...

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> I don't think we really want to take this before branching...

Well, effectively the patch just adds the console message, informing developers about what was already the case. The parameter doesn't work either way.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

It just seems kind of obnoxious to start throwing up alert dialogs in betas for something that doesn't really matter all that much, particularly after beta 7 when addon-affecting changes are supposed to be minimized.

Comment 7

[Dão Gottwald [:dao]]

I don't know that it doesn't matter much. I'm assuming that at least some developers intentionally set the parameter and wrote the listener in a way that causes bugs when being called unexpectedly.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I don't think it's likely that this is actually breaking things much in practice, because I don't think it's common for extensions to want to filter events to begin with, and because the broken behavior is consistent (it would occur on the developer's machine while testing just as frequently as it will for users). The only reason I didn't notice this while writing my test is that that listener removed itself after a single load was triggered, which I'm guessing isn't a common pattern. Once I changed it to be longer-lived the problem was immediately apparent.

So the only real upside to this that I see is cleanup (as opposed to extension-fixing), and that doesn't really outweigh the downside of noisy NS_ASSERT()s being triggered for cases that might not even have behavioral impact (e.g. someone passing NOTIFY_ALL). I'm not sure it matters much either way, though, so if you want to land a version for 4.0 that just reportError()s rather than NS_ASSERT()ing, I guess I'll approve it.

Comment 9

[Dão Gottwald [:dao]]

Attachment: updated to tip

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/cedar/rev/cd6080e94136

Comment 11

[Dão Gottwald [:dao]]

also:
http://hg.mozilla.org/projects/cedar/rev/627840006eb9
http://hg.mozilla.org/projects/cedar/rev/540cdd61fa9f

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/cd6080e94136
http://hg.mozilla.org/mozilla-central/rev/627840006eb9
http://hg.mozilla.org/mozilla-central/rev/540cdd61fa9f

Comment 13

[Gary [:streetwolf52]]

After installing changeset http://hg.mozilla.org/mozilla-central/rev/ab95ab9e389b
I get an error message 3 times before Fx4.2 starts.  I've attached the first message.  All the others are the same expect the number after the Object is different.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110323 Firefox/4.2a1pre - Build ID: 20110323175018

Comment 14

[Gary [:streetwolf52]]

Attachment: Error message

Comment 15

[Gary [:streetwolf52]]

SafeMode works OK.  Time to figure out what's causing the error.

Comment 16

[Dão Gottwald [:dao]]

https://developer.mozilla.org/en/XUL/tabbrowser#m-addProgressListener updated

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #15)
> SafeMode works OK.  Time to figure out what's causing the error.

Must be an add-on. Please notify the author.

Comment 18

[Gary [:streetwolf52]]

Will do now that I know which 3 add-ons are causing the message.

Comment 19

[DB Cooper]

Doesn't do this with a clean profile. 

Noscript (2.1.0rc5) is triggering the error message for me.

Comment 20

[Leon Sorokin]

yep, Noscript 2.0.9.9 and Firebug 1.7.0

Comment 21

[=Haron=]

Confirm the existence of a bug
OS Ubuntu 10.04.2 x86_64
:::: Mozilla/5.0 (X11; Linux x86_64; rv:2.2a1pre) Gecko/20110324 Firefox/4.2a1pre #1300972673

Comment 22

[=Haron=]

Источники бага - NoScript 2.0.9.9; Firebug 1.7X.0b4; Stylish 1.1.1

Comment 23

[John J. Barton]

Really? You put up an assertion dialog for something that you say never worked anyway?  Maybe Firebug should do that for all the Firefox things that don't work.

Comment 24

[Ken]

In tabbrowser.xml is it possible to display the the name of the calling script
on the error console, instead of using NS_ASSERT to popup a window that has to
be dismissed? (~> 30 warning popup's are a lot to be closed before Minefield
starts).

If NS_ASSERT has to be called can the caller name be displayed there, then at
least the user can figure out which add-on or Mozilla code needs the bug filed
for it.

The following example error console warning does not help figure out where the
error is occuring (who is calling addProgressListener, with invalid
parameters).
Warning: Unknown property '-moz-background-clip'.  Declaration dropped.
Source File:
https://developer.mozilla.org/en/XUL/tabbrowser#m-addProgressListener
Line: 0

Comment 25

[=Haron=]

I only give information about the addons causing this error
NoScript 2.0.9.9
Firebug 1.7X.0b4
Stylish 1.1.1
WOT 20110323
Scriptish 0.19b
Mozilla Archive Format 1.0a1
Maybe this will help

Comment 26

[John J. Barton]

The cause of the error is this bug patch: it should have thrown an exception not put up a dialog.

Firebug 1.8 has changed three calls to addProgressListener:
http://code.google.com/p/fbug/source/detail?r=9790
Of course we can't fix your already-installed 1.7

Comment 27

[vtol]

Calomel add-on is impacted all the same, W7 64b | 4.2a1pre 2011-02-24 64b

Comment 28

[rob64rock]

I only give information about the another Add-on causing this assertion dialog error:

Feeling Lucky Fixer 2.1 and 2.2 : https://addons.mozilla.org/firefox/addon/feeling-lucky-fixer/

Also can confirm:

WOT
Status-4-Evar
Stylish

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #23)
> Really? You put up an assertion dialog for something that you say never worked
> anyway?

Yep.

> Maybe Firebug should do that for all the Firefox things that don't work.

If Firefox erroneously expects them to work, then absolutely, please let us know about these things.

(In reply to comment #26)
> The cause of the error is this bug patch: it should have thrown an exception
> not put up a dialog.

An exception would have prevented affected add-ons from working in Firefox 5, whereas NS_ASSERT will just add a note to the error console.

Comment 30

[zigboom]

Sorry guys, I cannot offer a solution, I'm a Theme developer (of LavaFox & BlackFox). 
Since today I see this annoying error message, using Minefield 4.2a1pre. 
I have to say OK twice and then it works as normal. 
What's the point to make the user click OK when he cannot do anything about it? 
It's a major annoyance for nothing. Why not have a sliding message that disappears (such as for ready updates). What is the user saying OK to? This is like Chinese to 99.99% of the users, why should we say OK? (twice) just to start the browser? 
Sorry guys, I never complained but I love Firefox and hate to see it becoming annoying.

Comment 31

[Rob Madden]

Can we get rid of the box its so annoying. I dont like having to deal with it when I am a user not a develpoer of the add ons.

Comment 32

[John J. Barton]

(In reply to comment #29)
> (In reply to comment #26)
> > The cause of the error is this bug patch: it should have thrown an exception
> > not put up a dialog.
> 
> An exception would have prevented affected add-ons from working in Firefox 5,
> whereas NS_ASSERT will just add a note to the error console.

But the observed behavior is a popup dialog, not a note in the error console. That is why we are all complaining.

Comment 33

[Rob Madden]

(In reply to comment #32)
> (In reply to comment #29)
> > (In reply to comment #26)
> > > The cause of the error is this bug patch: it should have thrown an exception
> > > not put up a dialog.
> > 
> > An exception would have prevented affected add-ons from working in Firefox 5,
> > whereas NS_ASSERT will just add a note to the error console.
> 
> But the observed behavior is a popup dialog, not a note in the error console.
> That is why we are all complaining.

Finally someone got it right and pointed out its a popup window we are complaining about.

Comment 34

[ffextensionguru]

Am getting this now on: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.2a1pre) Gecko/20110324 Firefox/4.2a1pre ID:20110324030205. Also got something similar on Ubuntu as well. These are the add-ons I am using:
ChromEdit Plus 2.9.0
Photobucket Uploader 1.3
OpenDownload 1.0.0
CoLT 2.5.1
Greasemonkey 0.9.1
PDF Download 3.0.0.2
Rotate Image 0.1.3.2
SearchLoad Options 0.6.3
Adblock Plus Pop-up Addon 0.2.2 
Pixlr Grabber 2.1.1
Download Statusbar 0.9.8
Test Pilot 1.1 
IE Tab Plus 1.99.20110227 
bug582139(Allow bookmarks button in the nav bar)1.4 
Nightly Tester Tools 3.1.2 
Domain Details 2.6.9
Brief 1.5.3
Adblock Plus 1.3.3true

Comment 35

[Ed Morley [:emorley]]

Reopening given Dao's comment:

(In reply to comment #29)
> An exception would have prevented affected add-ons from working in Firefox 5,
> whereas NS_ASSERT will just add a note to the error console.

...implies that the intent of this bug was to make an error show in the error console; whereas in fact the result was 3 x dialogue popups on startup, before the main Fx window shows. If this was actually expected (and comment 29 misworded), then my apologies please mark fixed again.

In case there is any doubt, this is what is being shown:
https://bugzilla.mozilla.org/attachment.cgi?id=521461
...*not* an error console message.

This is being triggered by addons as popular as Noscript, Firebug and so on; and whilst it is their responsibility to fix in the long term; for now it makes using the nightlies quite annoying - and so it would be nice to have this as an error console message rather than a dialog box on startup.

Comment 36

[Rob Madden]

I've fixed and uploaded(to Mediafire) the beta version of Greasemonkey: http://www.mediafire.com/file/2zg1i3e7j7fpam0/greasemonkey-2011.03.13.beta.xpi

Comment 37

[ffextensionguru]

(In reply to comment #36)
> I've fixed and uploaded(to Mediafire) the beta version of Greasemonkey:
> http://www.mediafire.com/file/2zg1i3e7j7fpam0/greasemonkey-2011.03.13.beta.xpi

Installed, not getting error but now the scroll bar on the right is gone replaced by black space and part of the toolbars are missing as well. Context menus also not working correctly. Doing this both on the 32-bit and 64-bit 4.2a1pre on Windows 7.

http://i104.photobucket.com/albums/m195/ffextensionguru/firefoxdisplayissues.png

Comment 38

[ffextensionguru]

(In reply to comment #37)
> (In reply to comment #36)
> > I've fixed and uploaded(to Mediafire) the beta version of Greasemonkey:
> > http://www.mediafire.com/file/2zg1i3e7j7fpam0/greasemonkey-2011.03.13.beta.xpi
> 
> Installed, not getting error but now the scroll bar on the right is gone
> replaced by black space and part of the toolbars are missing as well. Context
> menus also not working correctly. Doing this both on the 32-bit and 64-bit
> 4.2a1pre on Windows 7.
> 
> http://i104.photobucket.com/albums/m195/ffextensionguru/firefoxdisplayissues.png

Actually still getting error on 64-bit version (although error window is nearly blacked out). 

http://i104.photobucket.com/albums/m195/ffextensionguru/assertion20failed.png

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.2a1pre) Gecko/20110324 Firefox/4.2a1pre ID:20110324183212

Comment 39

[ffextensionguru]

Reverted back to prior version of GreaseMonkey no change. Ran above version of Firefox with a fresh profile same behaviour (but no error). Ran 32-bit in SafeMode no error and display was fine.

Comment 40

[Rob Madden]

Found the latest Nightly for Greasemonkey this one should work this time. It did not give me the popup. https://arantius.com/misc/gm-nightly/greasemonkey-2011.03.17.nightly.xpi

Comment 41

[vtol]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > I've fixed and uploaded(to Mediafire) the beta version of Greasemonkey:
> > > http://www.mediafire.com/file/2zg1i3e7j7fpam0/greasemonkey-2011.03.13.beta.xpi
> > 
> > Installed, not getting error but now the scroll bar on the right is gone
> > replaced by black space and part of the toolbars are missing as well. Context
> > menus also not working correctly. Doing this both on the 32-bit and 64-bit
> > 4.2a1pre on Windows 7.
> > 
> > http://i104.photobucket.com/albums/m195/ffextensionguru/firefoxdisplayissues.png
> 
> Actually still getting error on 64-bit version (although error window is nearly
> blacked out). 
> 
> http://i104.photobucket.com/albums/m195/ffextensionguru/assertion20failed.png
> 
> Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.2a1pre) Gecko/20110324
> Firefox/4.2a1pre ID:20110324183212

seeing the same blackening stuff on W7 SP1 b64. something going horribly wrong with this one

Comment 42

[Yannick Sandoz]

I'm on mac OSX 10.6.8

Firefox/4.2a1pre 32-bit version

There's no problem with the scrolling bar on the right, just the warning message on launch.

Comment 43

[avada]

Of course the error console is useless...
God forbid anyone could actually identify which addons cause the annoying triple popup dialog to appear. Of course that is useless too, it just points here.

Comment 44

[Ed Morley [:emorley]]

Dao, please see comment 35.

Comment 45

[Brandon Sterne (:bsterne)]

FYI, I hit this right away with a new profile after installing Add-on Compatibility Reporter:
https://addons.mozilla.org/en-US/firefox/addon/add-on-compatibility-reporter/

Comment 46

[Daniel Veditz [:dveditz]]

There are hundreds of addons doing it the "wrong" way. Grepping through addons source for "gBrowser.addEventListener\(.*," found over 500 instances.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

Bug 644645 changed the NS_ASSERT to a console warning.

Comment 49

[Gabriela [:gaby2300]]

I'm getting a window alert refering to this bug on:

Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110326 Firefox/4.2a1pre

Comment 50

[chris hofmann]

also eventually figured out this was behind the problem I encountered in bug Bug 650858

Comment 51

[chris hofmann]

comment 46 and my experience in comment 50 really makes me want suggest we disable this or back it out for firefox 5 so we have at list on or more full development cycles for addon developers to respond.

Comment 52

[chris hofmann]

cc'ing jorgev to see if he thinks the outreach and updates can happen faster so this possible could stay on the firefox 5 train.

Comment 53

[Dão Gottwald [:dao]]

(In reply to comment #50)
> also eventually figured out this was behind the problem I encountered in bug
> Bug 650858

You're hastily connecting these two bugs. An add-on triggered this message? Yes, we know many add-ons will do that. The add-on also breaks Firefox? Quite possible, but likely not connected to the error message.

Comment 54

[chris hofmann]

what do we need to prove or disprove the connection, for the perspectives addon,  or any of other addons in dveditz' query?  

some hasty decisions, or a lot of debugging work, is probably in order to figure out a path to not shipping fx5 with possible major incompatibilities; especially given the low volume of aurora testers we have.

I guess one way to test the particular problem I saw is to back out your change from aurora, re-install perspectives, and see if the restart problem I saw goes away.

Comment 55

[chris hofmann]

the testing matrix for this is rather large even if only restricted to just the addons served off AMO.  Here is the list of 400+ addons that dveditz' query turned up in his mxr search in commment 46.

https://crash-analysis.mozilla.com/chofmann/bug608629.html

It seems like just have speculation about how these addons might be affected, and not much hard data and testing one way or the other.  

Rapid Release Cycles require that we understand the impact of changes early in the cycle and not later. we are about 1/2 way though 5.0 and still don't have adequate testing eyeballs on aurrora builds to understand the impact of the changes in that release.

Comment 56

[Dão Gottwald [:dao]]

(In reply to comment #55)
> the testing matrix for this is rather large even if only restricted to just the
> addons served off AMO.  Here is the list of 400+ addons that dveditz' query
> turned up in his mxr search in commment 46.
> 
> https://crash-analysis.mozilla.com/chofmann/bug608629.html
> 
> It seems like just have speculation about how these addons might be affected,
> and not much hard data and testing one way or the other.  

"Affected" just means they'll get the message in the error console. The message is non-disruptive and doesn't affect how these add-ons interact with Firefox.

Comment 57

[Kai Engert (:KaiE:)]

This bug should include a code snippet, that shows how to query the gecko version at runtime, and call the function with either two or one parameter, in order to allow addons that want to be compatible with both ff 4 and ff 5, and at the same time avoid the message on the error console.

Comment 58

[Dão Gottwald [:dao]]

The second parameter doesn't actually work in Firefox 4.

Comment 59

[Kai Engert (:KaiE:)]

(In reply to comment #58)
> The second parameter doesn't actually work in Firefox 4.

Ok, I see. I had assumed the function call were routed directly to C++, and using one parameter in FF 4 would either throw an exception or use undefined data.

Now I see this was implemented in XML, and the second parameter was ignored.

I conclude the instruction for addon authors is:

       Simply remove the second parameter.

Comment 60

[Dão Gottwald [:dao]]

If you're passing NOTIFY_ALL as the second parameter, just remove it.

If you're passing something else, it depends on what your code actually wants. If it wants only certain progress notifications, it's broken. If it actually wants all progress notifications, the second parameter was used wrongly and can be removed without a replacement.

Comment 61

[Zbynek Michl]

Hey, I removed the second argument and the error disappeared on Linux (that's correct). However, on Windows I'm still getting "gBrowser.addProgressListener was called with a second argument...". Both FF versions are from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/5.0b6-candidates/build1/. What's wrong?

Comment 62

[Zbynek Michl]

Forget my previous comment. The error was caused by another extension that I had installed in my profile :)

Comment 63

[Matěj Cepl]

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers