Closed Bug 608628 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 5

People

(Reporter: Gavin, Assigned: dao)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

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.
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.
(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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #487308 - Flags: review?(gavin.sharp)
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...
Attachment #487308 - Flags: review?(gavin.sharp) → review+
(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.
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.
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.
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.
Attached patch updated to tipSplinter Review
Attachment #487308 - Attachment is obsolete: true
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
Attached image Error message
SafeMode works OK.  Time to figure out what's causing the error.
(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.
Will do now that I know which 3 add-ons are causing the message.
Doesn't do this with a clean profile. 

Noscript (2.1.0rc5) is triggering the error message for me.
yep, Noscript 2.0.9.9 and Firebug 1.7.0
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
Источники бага - NoScript 2.0.9.9; Firebug 1.7X.0b4; Stylish 1.1.1
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.
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
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
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
Calomel add-on is impacted all the same, W7 64b | 4.2a1pre 2011-02-24 64b
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
(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.
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.
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.
(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.
(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.
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
Depends on: 644860
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I've fixed and uploaded(to Mediafire) the beta version of Greasemonkey: http://www.mediafire.com/file/2zg1i3e7j7fpam0/greasemonkey-2011.03.13.beta.xpi
(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
(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
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.
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
(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
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.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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.
Dao, please see comment 35.
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/
There are hundreds of addons doing it the "wrong" way. Grepping through addons source for "gBrowser.addEventListener\(.*," found over 500 instances.
Depends on: 644645
No longer depends on: 644860
Bug 644645 changed the NS_ASSERT to a console warning.
Target Milestone: --- → Firefox4.2
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
Target Milestone: Firefox5 → Firefox 5
also eventually figured out this was behind the problem I encountered in bug Bug 650858
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.
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.
(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.
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.
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.
(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.
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.
The second parameter doesn't actually work in Firefox 4.
(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.
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.
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?
Forget my previous comment. The error was caused by another extension that I had installed in my profile :)
Depends on: 684016

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
No longer depends on: 684016
You need to log in before you can comment on or make changes to this bug.