The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 5

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Gavin, Assigned: dao)

Tracking

({dev-doc-complete})

Trunk
Firefox 5
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 3

7 years ago
Created attachment 487308 [details] [diff] [review]
patch
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+
(Assignee)

Comment 5

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 521149 [details] [diff] [review]
updated to tip
Attachment #487308 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/projects/cedar/rev/cd6080e94136
Whiteboard: fixed-in-cedar
(Assignee)

Comment 11

6 years ago
also:
http://hg.mozilla.org/projects/cedar/rev/627840006eb9
http://hg.mozilla.org/projects/cedar/rev/540cdd61fa9f
http://hg.mozilla.org/mozilla-central/rev/cd6080e94136
http://hg.mozilla.org/mozilla-central/rev/627840006eb9
http://hg.mozilla.org/mozilla-central/rev/540cdd61fa9f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar

Comment 13

6 years ago
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

6 years ago
Created attachment 521461 [details]
Error message

Comment 15

6 years ago
SafeMode works OK.  Time to figure out what's causing the error.
(Assignee)

Comment 16

6 years ago
https://developer.mozilla.org/en/XUL/tabbrowser#m-addProgressListener updated
Keywords: dev-doc-complete
(Assignee)

Comment 17

6 years ago
(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

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

Comment 19

6 years ago
Doesn't do this with a clean profile. 

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

Comment 20

6 years ago
yep, Noscript 2.0.9.9 and Firebug 1.7.0

Comment 21

6 years ago
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

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

Comment 23

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

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

Comment 28

6 years ago
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
(Assignee)

Comment 29

6 years ago
(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

6 years ago
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

6 years ago
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

6 years ago
(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

6 years ago
(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

6 years ago
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 → ---

Comment 36

6 years ago
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

6 years ago
(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

6 years ago
(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

6 years ago
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

6 years ago
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

6 years ago
(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

6 years ago
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.
(Assignee)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 43

6 years ago
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.
(Assignee)

Updated

6 years ago
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
(Assignee)

Updated

6 years ago
Target Milestone: Firefox5 → Firefox 5

Comment 50

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

Comment 51

6 years ago
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.
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?

Comment 52

6 years ago
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.
(Assignee)

Comment 53

6 years ago
(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.
tracking-firefox5: ? → ---
tracking-firefox6: ? → ---

Comment 54

6 years ago
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

6 years ago
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.
(Assignee)

Comment 56

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 58

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

Comment 59

6 years ago
(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.
(Assignee)

Comment 60

6 years ago
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

6 years ago
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

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

Updated

6 years ago
Depends on: 684016

Comment 63

4 years ago

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

Updated

4 years ago
No longer depends on: 684016
You need to log in before you can comment on or make changes to this bug.