816003 - Callback function of setAndLoadFaviconForPage in FF18 beta 1 do not work

Status: RESOLVED INVALID

(Toolkit :: Places, defect)

Description

[Sonthakit]

Attachment: ForCheck.json

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121119183901

Steps to reproduce:

Fresh Firefox 18, install add-on bookmarkfaviconchanger
Restore bookmark that have favicon data embed
(example in attach file)




Actual results:

In Firefox 18 beta 1, it is very slow.


Expected results:

First, I am the author of bookmarkfaviconchanger myself.
I find that FF 18b1 is very slow when restore favicon. (Restore funciton is in restore.js in bookmarkfaviconchanger)
Explain the process in restore.js is...
I have multiple favicon to restore. I need one by one favicon to restore to make UI not freeze. I restore one favicon, --> if success, call backfunction will call and it will restore the next favicon. If callback function not call, it mean that something error. The timeout function 10 second will call for next favicon to restore.
If you use FF17, the callback function will call every time making process quick. But in FF18 beta 1, the restore is very slow due to no callback function is call. All of process depend on timeout 10 second.
Please check and fix. If you want more information, please email me at [e-mail]

Comment 1

[Sonthakit]

More ERROR
If you do not specified call back function, it will return exceptional.
[Exception... "Not enough arguments [nsIFaviconService.setAndLoadFaviconForPage]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"

This is not good. Call back function is OPTIONAL argument!!
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIFaviconService#setAndLoadFaviconForPage%28%29

Comment 2

[Loic]

Did you test with Firefox 19 and 20?

In addition, could you write clear STR: after the add-on is installed in the new profile, what should the user do?

Comment 3

[Sonthakit]

OK. Forget about everything. Make it simple from begining.
I make a new simple add-on. It just simple call the funciton setAndLoadFaviconForPage
It will change the default bookmark "getting start" which is at URL http://www.mozilla.com/en-US/firefox/central/
to use google icon "http://www.google.com/favicon.ico"

Please download and install from
http://dl.dropbox.com/u/7679101/testbug816003.xpi
(If anyone can attach this file to this page, it will OK. I don't know how to attach it)

The code in this add-on is 

var testbug816003 = {
  onLoad: function() {
  var ioservice = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
  var pageURI = ioservice.newURI("http://www.mozilla.com/en-US/firefox/central/", null, null);
  var faviconURI = ioservice.newURI("http://www.google.com/favicon.ico", null, null);
  Components.classes["@mozilla.org/browser/favicon-service;1"].getService(Components.interfaces.nsIFaviconService).setAndLoadFaviconForPage(pageURI, faviconURI, false); 
  },
};
window.addEventListener("load", function () { testbug816003.onLoad(); }, false);

Testing in FF17 En-US, fresh, new install new profile
Result --> default bookmark "getting start" change to google icon, no error.

FF18b1 En-US
Error: NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsIFaviconService.setAndLoadFaviconForPage]
Source File: chrome://testbug816003/content/ff-overlay.js
Line: 6

FF19a2 En-US
Timestamp: 29/11/2555 0:57:15
Error: NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsIFaviconService.setAndLoadFaviconForPage]
Source File: chrome://testbug816003/content/ff-overlay.js
Line: 6

And I believe, if you add callback function, it will not call.

Comment 4

[Loic]

I'm not sure if it's a valid bug anyway here is the regression range:

m-c
good=2012-09-18
bad=2012-09-19
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0d3b17a88d5f&tochange=80499f04e875

Suspected bug:
Ehsan Akhgari — Bug 741059 - Part 3: Update the callers to nsIFaviconService::SetAndLoadFaviconForPage; r=jdm

Comment 5

[Loic]

Attachment: Reporter's extension (.xpi)

Comment 6

[Marco Bonardo [:mak]]

setAndLoadFaviconForPage is deprecated... btw the signature of the method changed, the callback is the 5th argument, while before it was the 4th.

Comment 7

[Sonthakit]

Why do this?
If you change the setAndLoadFaviconForPage from bug 686032
Adding the new 4th argument and change old 4th argument to 5th argument
This will cause a lot of extension to crash.
If you need a private browsing channel boolean, why not add it to 5th argument and make it optional?
Optional default should be true or fault depend on current private browsing status.

This make add-on developer a very difficult situation.
You must write the code for FF17 and before
and you much write the second code for FF18 and after

Comment 8

[(no longer active)]

The reason that we had to do this is that the callback argument is optional, but aFaviconLoadType is not, so we couldn't put that after the optional argument...

Comment 9

[Sonthakit]

Then make it optional and put it in 5 argument.
Why it cannot optional? Tell me one good reason.

You're making firefox unstable and push the user to use other browser.
User need stability.
This change make every extension that use this function crash.
You should make everything compatible with old version as much as possible.

Comment 10

[Sonthakit]

Summary.

You change 
setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, [optional] aCallback);
into
setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, aFaviconLoadType, [optional] aCallback);

This will make every add-on that use this function crash.
If add-on not specified 4 argument, it will return NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments.
And if add-on use aCallback function as argument 4, it will not callback anymore because argument 4 turn into aFaviconLoadType

More... This will make programmer in difficult situation because every time you try to use this function, you must write

if (Firefox version < 18) {
   setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, [optional] aCallback);
} else {
   setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, aFaviconLoadType, [optional] aCallback);
}

It is very inconvenience. And I can expected that if FF 18 ship off with this feature. A lot of non-working add-on and crash will report.

What you should do is make aFaviconLoadType is a 5th and optional argument
   setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, [optional] aCallback, [optional] aFaviconLoadType);

You can make default value of aFaviconLoadType in different ways
1) default is always true
2) default is always false
3) default can be true of false depend on situation (system selected)

That its!!

Comment 11

[Marco Bonardo [:mak]]

if you use this from cpp you should get an error when building, otherwise I don't see why you would be crashing, wouldn't it throw at a maximum? aFaviconLoadType is an int and in the worst case you are passing a function to it.

The original bug has been marked as addon-compat, AMO should mark extensions using the old signature as incompatible and mail all authors, afaik.
Private browsing is changing, that means most add-ons will have to comply to new rules, there's unfortunately no way around this, expecting add-ons to be "ready" out of the box with optional arguments is likely not going to work properly.  If your add-on would add icons loaded from a pb window you'd be hitting the user's privacy.

Comment 12

[Sonthakit]

No I don't agree. It 100% crash.

If you call this function only 3 argument, it will cause exception in FF18 - NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments.
and if you pass function callback in argument 4, it will not work. Function will not call.

This will absolutely making all add-on that use this function FAIL !!! no mater what how much arguments or what is argument

Other thing...
I think you only have two value in aFaviconLoadType
// The favicon is being loaded from a private browsing window
const unsigned long FAVICON_LOAD_PRIVATE = 1;
// The favicon is being loaded from a non-private browsing window
const unsigned long FAVICON_LOAD_NON_PRIVATE = 2;

It don't make sense that you can't make the default value.
Let me say
you should set this value to 1 if FF is in private browsing mode.
you should set this value to 2 if FF is in non-private browsing mode.
you should set this value to 1 if you don't know

SO.... EXPLAIN ME WHY YOU CAN'T MAKE DEFAULT VALUE !!!

It is a bad behavior, changing function to make old software crash.
If you force to do, you should make new function "setAndLoadFaviconForPage2"

And you say, Private browsing is going to have new rules.
What rules? Any developer hear? Anything tell him?
You're just tell him that it is incompatible and the next 4 day you ship the new FF.
Developer have 4 day while you have 6 weeks !!! DON'T SICK, DON'T DIE, DON'T GOTO HOLIDAY
IF THE USER CRASH, IT IS THE FAULT OF ADD-ON DEVELOPER. BLAR BLAR BLAR

Comment 13

[Marco Bonardo [:mak]]

Sorry, I still don't see the crash evidence, as you said it causes an exception, and the add-on doesn't work. I'm sorry about that, but I highly doubt this is going to change, yous suggestion of making the argument optional would make it optional in the stricter way, thus considering it always in pb unless you specify it's not. Then your add-on would still be broken. The opposite couldn't be done since it would end up being a privacy hit for the user, that is what both us and you care about most.
Notice that Private browsing will be PER WINDOW, so there is not evidence we can just say "we are in pb" or "we are not in pb" it is the caller responsibility to tell us.

I take your point that the notification of incompatilibities could be sent too late in the development cycle (I honestly don't know), than maybe this could be considered an actual thing to be discussed and changed, thus I'm cc-ing Jorge, so maybe we can get a clarification of how the incompatilibities notifications are sent and how much time before.

Comment 14

[Sonthakit]

Not agree.
Why my add-on will broke if argument default is in "private" mode?
If it request favicon in private mode, let it be. It is no effect to my add-on as long as I can set favicon !!

And you're going to say private browsing is per window.
You forget something?

My code call this function in many ways. I call in module environment (no window). It call in bookmark sidebar. I call in bookmark manager. It call in my add-on XUL. And most difficult is... I call it after "quit-application-grant" to clear favicon if user uninstall my add-on. You see?

Tell my why it is not work if default is "always private browsing"

Another thing. If you really really want to do this.
MAKE NEW FUNCTION - don't destroy the old one.
When compatibility check - tell developer that "WARNING - THIS FUNCTION WILL REMOVE IN FUTURE, PLEASE CHANGE TO THE NEW FUNCTION".
Give him some time, like 3-6 month before remove the old function.

Comment 15

[(no longer active)]

Sonthakit, I understand that you're very frustrated, and it's definitely not great to see the code that used to work before suddenly break.  The fact of the matter is that we change things all the time in Gecko, and sometimes it's not possible to keep the old APIs working, because the concepts that used to exist before would change.  This is an example of such a case, the global private browsing mode is going away, and that is the reason why we cannot come up with a default value for this new argument and make it optional.  Starting with Firefox 18, it will be the responsibility of every single caller of this function to decide whether they want the favicon to be loaded in private browsing mode or in normal mode.  For the places that you have a window around, you can call PrivateBrowsingUtils.isWindowPrivate on the window object to determine whether the window is in private mode.  For the places that you don't have a window around, you need to decide whether loading the add-on can expose user's browsing history or not.  If it can, you should load it in private mode, otherwise in non-private mode.

Hope this helps.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

I don't see anything actionable to track here, this was resolved invalid (and likely will be again when discussion completes its course).

Comment 17

[Sonthakit]

I ASK THAT WHY DON'T DO THE DEFAULT TO BE ALWAYS PRIVATE.
No one answer.

I ASK THAT WHY DON'T DO THE NEW FUNCTION SetAndLoadFaviconForPage2 for your new argument, let the SetAndLoadFaviconForPage USE ALWAYS PRIVATE (at least for a while for stability)
No one answer.

Changing the argument of function is unacceptable for API programmer. A good API programmer will make the old one obsolete and make a new function. API function must fully compatible with old software.

You just answer -- I want to do this. No matter what there is a way not to break old add-on. No matter what there is a better way. I just don't want to do this. I do this already. No one change my code. Don't need to care add-on developer. If FF crash then say together that "IT IS BAD ADD-ON". Don't discuss this topic anymore, it is invalid. BLAR BLAR BLAR

You all don't understand the situation. This rapid cycle is bad. Everytime FF upgrade the version, it worse. No one care about -- FF faster, FF use less memory, FF has a new private browsing mode. But instead, a lot of say - CRASH AGAIN WHEN FF UPGRADE. Many user change to other browser because it crash and crash. Many time you need to release rapid fix version and that time you loss the user again. 

More and more I see a good developer quit. I see a good add-on disappear. Many user quit FF because their favorite add-on not work. Just because situation like this. Anyway, I think that Add-on developer is a second class citizen, same as the quit developer of "CheckPlaces" say
http://forums.mozilla.org/addons/viewtopic.php?p=24858&sid=44b95822d7569aba11942b1a6983690f

UNNECESSARY INTENTION TO BREAK PREVIOUS ADD-ON. I cannot accept this. May be I should quit and let the all user of my extension crash in FF18. No hope for this FF any more.

UNNECESSARY UNACCEPTABLE INTENTION TO BREAK PREVIOUS ADD-ON
UNNECESSARY UNACCEPTABLE INTENTION TO BREAK PREVIOUS ADD-ON
UNNECESSARY UNACCEPTABLE INTENTION TO BREAK PREVIOUS ADD-ON
UNNECESSARY UNACCEPTABLE INTENTION TO BREAK PREVIOUS ADD-ON
UNNECESSARY UNACCEPTABLE INTENTION TO BREAK PREVIOUS ADD-ON

Comment 18

[Loic]

Sonthakit, could you stop your pointless sentences in all caps, it's rude and not very useful to the discussion, thanks.
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 19

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Sonthakit from comment #17)
> I ASK THAT WHY DON'T DO THE DEFAULT TO BE ALWAYS PRIVATE.
> No one answer.

As I was informed, if we default to private, the favicon won't be stored, which means the function will silently fail in a much more subtle way that will confuse developers even more.

> I ASK THAT WHY DON'T DO THE NEW FUNCTION SetAndLoadFaviconForPage2 for your
> new argument, let the SetAndLoadFaviconForPage USE ALWAYS PRIVATE (at least
> for a while for stability)
> No one answer.

For the same reason as above.

> Changing the argument of function is unacceptable for API programmer. A good
> API programmer will make the old one obsolete and make a new function. API
> function must fully compatible with old software.

Firefox APIs aren't handled as strictly as, say, Java APIs. These APIs are mostly meant to service Firefox, so they are changed as Firefox development requires it. They aren't changed gratuitously, and we evaluate add-on impact when they are changed. In this case, the impact is relatively small (a few dozen possibly affected add-ons), and the fix is simple for you to implement.

This change won't be reverted, so it will be a more productive use of your time to implement the version check you suggested on comment #10.

Comment 20

[Sonthakit]

> As I was informed, if we default to private, the favicon won't be stored, which means the function will silently fail in a much more subtle way that will confuse developers even more.

Then if for the new function if  aFaviconLoadType = 1 (FAVICON_LOAD_PRIVATE) will fail too
setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, aFaviconLoadType, [optional] aCallback)

Why if you specified it to 1, it is not fail. But if you set the default of old function to always private, it fail.

So, explain me the effect of new function. If it is 1, what will occur to bookmark favicon.

What really happen if aFaviconLoadType = 1
The bookmark favicon will not set because there is no data store?

If aFaviconLoadType = 1 alway fail, then it force my extension to use value 2 in all case. Then it will make your private browsing window data leak 100% if you concern.

Look! In private browsing mode, you can bookmark URL and it is keep.
If the new one is window private browsing, if you keep that URL to bookmark, should favicon save?

So, explain me, why you can keep bookmark URL in private browsing but can't keep bookmark favicon.

Comment 21

[Sonthakit]

More... Compare to XMLHttpRequest
function XMLHttpRequest is automatic associate with windows that call it.
If user close that window, this function will terminate.

You can apply this to setAndLoadFaviconForPage
Associate this function to window, then you can decide that you need to private browsing or not
If setAndLoadFaviconForPage call from module environment, then make it "non private browsing"

Comment 22

[(no longer active)]

(In reply to comment #21)
> More... Compare to XMLHttpRequest
> function XMLHttpRequest is automatic associate with windows that call it.
> If user close that window, this function will terminate.

XMLHttpRequest associates its load with the load group of the parent document, if I'm not mistaken, and that's how it knows whether it should handle the load in private mode or not.

> You can apply this to setAndLoadFaviconForPage
> Associate this function to window, then you can decide that you need to private
> browsing or not
> If setAndLoadFaviconForPage call from module environment, then make it "non
> private browsing"

What you're suggesting is very hard to implement.  In fact, I don't know how to tell whether a function has been called from C++ without any Javascript being involed, from Javascript in a module environment, etc.  If you have a way to do this, please share the details.  Thanks!

Comment 23

[Sonthakit]

If XMLHttpRequest can associates its load with the load group of the parent document
Why can't setAndLoadFaviconForPage.
Thank

Comment 24

[Sonthakit]

This is my last comment for this topic. Please read. I will not bother you again.
 
I will request and explain something that making me uncomfortable for both this new function argument and rapid release cycle of Firefox.

1)  You should make add-on more space and easier to develop and maintenance. Give more time for developers to upgrade, test run and search for conflict for their add-on.

In developer center, you usually comment and suggest that developer should not do this method. Should do that method instead because it is more reliable, easier to debug and less conflict. Such as developer should not use eval() function. But when I comment that changing argument 4 of setAndLoadFaviconForPage cannot accept. You say that Firefox APIs aren't handled as strictly as Java APIs. These APIs are mostly meant to service Firefox, so they are changed as Firefox development requires it.

	So, I can say the same thing that it is not a good thing. You should make API function strictly as much as possible. I suggest that you make new function setAndLoadFaviconForPage2 for your new propose. And make the default of the old setAndLoadFaviconForPage to "not use private browsing". 

When scanning compatibility for Add-on, you can tell the developer "Warning - setAndLoadFavionForPage will be obsolete in next 3 months or in FF20. So please update your add-on." 

	Let's me say, developer is human. He can sick. He may need hospitalized for weeks. At this time in rapid cycle release, you run compatibility check before FF release just only 4-5 days before. And you email to them that their add-on are incompatible. How can developer do fixing the job in such a short time? They need to code, debug, test run, observe conflict and wait for AMO editor to approve. You need to give him more time. Example is "TabMixPlus" problem in FF17. Despite TabMixPlus is a VIP add-on , it still late. (Surely AMO will approve this VIP add-on in 1 second)

	For leakage data concerning if default of setAndLoadFaviconForPage set to "do not use private browsing". You say by yourself that not much add-on use this function. So the risk of information leakage is very low. And it is just a favicon, not high risk security information. Why just accept some rare insignificant leak for only 3 months exchange for more stability of previous add-on and more reliable API code.


2) Information is lack.

	I know that my add-on is crash in FF18 because one user email to me. And I am surprise that there is no information why it crash. Let's me said, there are no any document say that FF18 will be private browsing per window. There is no information say that setAndLoadFavionForPage change the 4th argument. Even in "FF18 for developers web site" and "Mozilla add-on blog" do not say anything.

https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers
https://blog.mozilla.org/addons/

How can other developers know this information. And FF18 will ship in just next 35 days. How can they know and fix thing in time.

3) You expect that new FF version + disable incompatible add-on is better than old FF + full function add-on. It is incorrect. Example is… user may in happiness while use some add-on in FF17. And when he upgrade to FF18, the add-on is incompatible. He will get a new "private browsing per window" which he never want to use. But he lost Add-on function. Some add-on may be very important because it save the sensitive information of user. User may never want to loss it.
 
So, what I suggest is to change the upgrading process. If user click the upgrade button - then upgrade process scan the add-on and plug in. Then prompt the user the summary that - New feature for FF18 has 1,2,3… blar blar blar. And in the second line tell the user that Add-on 1,2,3… blar blar blar is incompatible for FF18 and cannot use. Then let's user decide that he will OK or cancel the upgrade.

Why it is important. Because user thinks that this is a problem from bad developer. You can see? 4 day for fix bug, test run, observe conflict and wait for AMO editor to approve?  Is this OK for you?

----------------------
Thank for reading. I will not bother you any more for this bug ID.

Comment 25

[Jorge Villalobos [:jorgev] (he/him)]

We decided to keep the changes, so add-ons will need to adapt to the change. We will inform add-on developers through our compatibility communications and automatic validation.

Comment 26

[Sonthakit]

Something still not right.

It is about 1 weeks that FF18 had released.
But the page information of setAndLoadFaviconForPage is still not updage.

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIFaviconService

Also, some extensions that should not compatible with FF18 because it contain "setAndLoadFaviconForPage" are mark as compatible with FF18 (eg. Favicon Picker 2 and  identFavicon)

https://addons.mozilla.org/en-US/firefox/addon/favicon-picker-2/
https://addons.mozilla.org/en-US/firefox/addon/identfavicon/

Comment 27

[Josh Matthews [:jdm]]

Thank you for the reminder about the docs. I have updated them. Jorge, can you look into the addons mention in comment 26?

Comment 28

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Josh Matthews [:jdm] from comment #27)
> Thank you for the reminder about the docs. I have updated them. Jorge, can
> you look into the addons mention in comment 26?

Sure. All add-ons are marked as compatible by default, and it is up to developers to update them. We rely on user reports to identify incompatible add-ons and let the developers know they need to fix them. The developers should have received a message from us when we ran the automatic compatibility validation some time ago.