Closed Bug 820522 Opened 12 years ago Closed 11 years ago

calling nsIWebBrowserPersist.saveURI breaks hundreds of our Apps

Categories

(Firefox :: Extension Compatibility, defect)

Product:
Firefox
Component:
Extension Compatibility
Version:
18 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: sylvaing, Unassigned)

References

Details

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.95 Safari/537.11

Steps to reproduce:

Calling nsIWebBrowserPersist.saveURI breaks addons by creating an NS_ERROR_XPC_NOT_ENOUGH_ARGS exception. To what I understand, from Firefox 18, a new new parameter called aPrivacyContext as been added to this method. This parameter can be set to null. reference : https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIWebBrowserPersist#saveURI()

Is there any possibilities to get nsIWebBrowserPersist.saveURI to continue working with 6 parameters and to use aPrivacyContext as null if this parameter is not provided?

Otherwise, we have literally hundred of addons that will need be updated before the release of FF18 in around 26 days.


Actual results:

Error console : 

Timestamp: 2012-12-11 14:54:02
Warning: WARN addons.manager: Exception calling callback: [Exception... "Not enough arguments [nsIWebBrowserPersist.saveURI]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: chrome://mystartcv4tb/content/toolbar.xul :: <TOP_LEVEL> :: line 1"  data: no]
Source File: chrome://mystartcv4tb/content/toolbar.xul
Line: 1
Changed component to "Extension compatibility"
Component: Untriaged → Extension Compatibility
In other terms; is there a way this method could be backward compatible?
It will leak privacy information to pass null blindly.
Blocks: 794602
Yes, I understand passing null blindly could cause privacy issues. That being said, is there a way this method could be backward compatible without compromising the privacy of the user?
It break my add-on "bookmark favicon changer" also.
This bug cause my head spin for weeks due to I cannot find the error.
It is very difficult to debug if API function is not freeze !!!
Imagine that I much re-check every code line and reread all the API detail.

Without warning, without any message, Mozilla just add 7th non-optional argument making add-on crash.
I don't think that Mozilla will care for any non-VIP add-ons anymore.

This is the second function that cause bug like this. I am very angry for unstable API.
The situation is like the function nsiFaviconService.setAndLoadFaviconForPage that I have report for bug 816033

https://bugzilla.mozilla.org/show_bug.cgi?id=816003

The situation of nsIWebBrowserPersist.saveURI is better than nsiFaviconService.setAndLoadFaviconForPage
because the latter reference is still not update (13 Dec 2012)

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIFaviconService#setAndLoadFaviconForPage()
[shift 4th argument to 5th argument and add new 4th argument]

So, make up your mind.
And pray that there are no more any hidden unexpected bugs like this. :(
Correction
bug 816033 --> bug 816003
(In reply to Sonthakit from comment #5)

For now what we did on our side is to always add the 7th parameter and set it to null for all our Addons. It would seem that this works properly for Firefox 13+ (including Firefox 18 beta 3 and the current build of aurora (FF 19).

This at least fix the problem but now need to update hundreds of Add-ons out there and this will take a very long time!!

Is this a viable fix you can implement on your side Sonthakit? I'm not sure if you have one or multiple Addons to update on your side.
My fix in my add on is using if ... else ...

if (firefox version before 18) {
   nsIWebBrowserPersist.saveURI  use 6 paramenter;
} else {
   nsIWebBrowserPersist.saveURI  use 7 paramenter;
}

But for Mozilla (if they read this), I can suggest the solution as same as bug 816003. But I don't think they will change.

The simple is ... create new function saveURI2 which use 7 parameter
old saveURI still use only 6 parameter but will obsolete in FF20 (which have private browsing per window)

During this time to FF20 release, email the developer that saveURI will obsolete in FF20, please change to new function saveURI2. And add this waring in "error console"

This will make add-on adapt, and easy to debug.
(In reply to Sonthakit from comment #8)
> if (firefox version before 18) {
>    nsIWebBrowserPersist.saveURI  use 6 paramenter;
> } else {
>    nsIWebBrowserPersist.saveURI  use 7 paramenter;
> }
Passing always 7 paramenters didn't work? The last extra parameter will be ignored under Firefox <=17.
(In reply to Masatoshi Kimura [:emk] from comment #9)
> Passing always 7 paramenters didn't work? The last extra parameter will be
> ignored under Firefox <=17.

It works for us. The parameter seems to be properly ignored without throwing exception for Firefox <= 17.
The ``saveURI` has been changed.` error also came up in my validation results. What is the easiest solution? Should I just add an null or is that problematic for user privacy? If that is the solution, I don't see why it can't be the default without breaking anything. Anyways, should I just change
wbp.saveURI(uri, null, null, null, null, fileURL);
to 
wbp.saveURI(uri, null, null, null, null, fileURL, null);

I can also post the whole code.

Thanks!
Please see https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIWebBrowserPersist for more information. To preserve user privacy, null is not a good default.
Furthermore, see https://developer.mozilla.org/en-US/docs/Supporting_per-window_private_browsing for how to obtain an nsILoadContext argument.
Thanks.

So is this okay

    var nsILoadContext = null;
    try {
        Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
        var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator);
        var win = wm.getMostRecentWindow("navigator:browser");
        nsILoadContext = PrivateBrowsingUtils.getPrivacyContextFromWindow(win);
    }
    catch(err) {}

wbp.saveURI(uri, null, null, null, null, fileURL, nsILoadContext);
Maybe, assuming that the most recent browser window actually makes sense to use here. If there is an actual window object that can be associated with the URI being saved, that would be a much better argument for getPrivacyContextFromWindow. If your saveURI call is not called synchronously from an interaction with a browser window, you could have a situation where the user switches windows before the saveURI call is reached and the wrong privacy status is used.
saveURI is called from my plugin preference window when the user clicks "download bla". Should I use that window? How do I get it?
I'm going to need more context about your addon to properly answer that question, such as how the preference window is invoked and what sort of URIs are listed and being saved. However, it would be better to move this to a forum such as mozilla.dev.extensions (https://www.mozilla.org/about/forums/#dev-extensions), since this isn't the best place for such discussions.
Thanks! I posted my question with additional details here: 
https://groups.google.com/forum/#!topic/mozilla.dev.extensions/JaGveFTqK3Y
This API change apparently breaks the download feature (i.e., half the functionality) of S3 Fox Organizer (http://www.s3fox.net/) as well. A fix for backward compatibility does seem warranted by the wide range of affected plug-ins.
As previously explained, a backwards-compatible solution is not feasible because it leaves the user vulnerable to privacy leaks. Addons using this functionality need to be updated, and will be automatically flagged as such by the auto-compatibility updater.
An update to S3 Fox Organizer was released today to resolve this issue (http://www.s3fox.net/).
To repeat comment 3, please DO NOT update your add-ons to pass null as the sevenths arguments. I've rejected at least a dozen add-on submissions for this over the past couple of days. If there is a valid privacy context which can be passed for the seventh argument, it needs to be passed. If you are taking screenshots of a window, you need to pass a privacy context derived from that window. If you are downloading an image or a video from a window, likewise. In short, if what you are downloading is in any way related to a specific browser window, you need to pass a relevant privacy context.

I'm WONTFIXing this as a) it's already too late, and b) if there were a backward compatible way to do this, the interface would not have been changed to begin with.
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
I think I know why a lot of developers use "null" for last parameter.
Because the example in reference doc is incorrect.

Please look at
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIWebBrowserPersist

in the last example, line 24-26
var privacyContext = sourceWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                                     .getInterface(Components.interfaces.nsIWebNavigation)
                                     .QueryInterface(nsILoadContext);
 
This make error "ReferenceError: nsILoadContext is not defined"

It should be change to

var privacyContext = sourceWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                                     .getInterface(Components.interfaces.nsIWebNavigation)
                                     .QueryInterface(Components.interfaces.nsILoadContext);
Oof. Thank you for pointing that out and changing it.
From the nsiWebBrowserPersist doc
"Important: If you think that you should be passing null here, you are almost certainly wrong. null should be passed only when no plausible privacy context exists for the URI to be saved, which is an exceedingly rare corner case."

I guess we're that rare corner case.  We use the autoadmin.global_config_url for a custom corporate install which downloads a config file that can trigger a download. The download occurs on startup before any browser window is shown so there is no window context that can be obtained.   I had to use null for this parameter in our extension to fix it.
And that's totally fine. Welcome to the club of edge cases!
You need to log in before you can comment on or make changes to this bug.