769440 - Failing assertion Assertion failure: mInitialized, at ../../../widget/xpwidgets/nsTransferable.cpp:447 during test-clipboard.testGetImage

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=13078908&tree=Firefox

During this test:
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/tests/test-clipboard.js#L171-186

We are having following assertion failure:
info: executing 'test-clipboard.testGetImage'
WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x8000FFFF: file ../../../../content/base/src/nsContentUtils.cpp, line 2903
Assertion failure: mInitialized, at ../../../widget/xpwidgets/nsTransferable.cpp:447

You can reproduce it with:
$ cd ..../addon-sdk
$ source bin/activate
$ cd packages/addon-kit
$ cfx test -v -f test-clipboard -b .../path/to/firefox/debug/bin --no-run
This command will print a command to run thought gdb:
$ gdb --args ....

Comment 1

[Eddy Bruel [:ejpbruel]]

Relevant trace:
http://pastebin.mozilla.org/1684582

Comment 2

[Eddy Bruel [:ejpbruel]]

Based on the above trace, I do not think this is a platform bug. The fact that the assert happens within a call to js::InvokeKernel suggests that some function is being called from JavaScript that maps to the function nsTransferable::AddDataFlavor in C++ (i.e. this assertion is triggered by calling a function on a XPCOM object).

Alex, any ideas on what could be the cause of this?

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 474

We are facing this new assertion introduce in bug 722872,
as we are not calling the new `init` method on nsITransferable objects.
This method has been introduce for the new non-global private browsing mode,
so that we can pass a nsILoadContext object in Init in order to later determine if tha transferable data is private or not.
But it isn't really clear for me if we should pass a context in our usecase?

Ehsan: Do you think it is fine to pass `null` in `xferable.init(null)` in SDK code? We are using nsITransferable objects for clipboard API which allow to get/set clipboard values:
* In case of get, we are using :
  clipboardService.getData(
    xferable,
    clipboardService.kGlobalClipboard
  )
So that we don't really know from which document/context does the data comes from. And I don't think it make any value to set a context, as we just fetch its transfer data and drop it right after.

* In case of set, we do not have any meaningfull document/context to give. The only meaningfull context would be "this addon" but addons do not have nor context, nor document. I'm wondering if it would be usefull for addons developers to say: I'm setting a clipboard value and this value is private? And/Or would it be usefull to accept a document in clipboard.set method in order to allow addon developers to set a custom clipboard value and linked it to one specific tab private state?

Matteo: Your thoughts?

Comment 4

[Daniel Holbert [:dholbert]]

FWIW, I filed bug 769961 on an instance of this same assertion-failure when dragging and dropping a file onto a Firefox window.

Comment 5

[(no longer active)]

This was done in bug 722872.  See the various getLoadContext() helper methods in the part 1 patch there for an example of how to get the appropriate load context to be passed to the init method.

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> This was done in bug 722872.  See the various getLoadContext() helper
> methods in the part 1 patch there for an example of how to get the
> appropriate load context to be passed to the init method.

Did you read comment 3? From what I understood about this new private browsing feature, I do not have any meaningfull window to use for this getLoadContext helper method. I can use the current toplevel chrome/content window, but I'm not sure it make any sense for addons usage.
  function getLoadContext() {
    const Ci = Components.interfaces; 
    return window.QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIWebNavigation)
                 .QueryInterface(Ci.nsILoadContext);
  }

Comment 7

[Josh Matthews [:jdm]]

For getting data from the clipboard, you can pass certainly null. For setting, it might make sense to modify the API to take an optional source window or document parameter.

Comment 8

[(no longer active)]

Yeah, pass null for reads.  For writes, the API needs to be modified.

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

I'm still not quite sure about the document we could use when a developer want to set an arbitrary string generated from an add-on, that aren't related to any window or document.

I'd like also avoid for the developer to set an arbitrary document or window reference when he set some strings to the clipboard, as additional parameter. Instead, I will suggest to modify the `Clipboard.set` to accept also a `selection` instance (our jetpack selection instance), so that we can automatically detect the document / window it belongs. 
An example of code could be:

    const clipboard = require("clipboard");

    require("selection").on("select", function(){
       clipboard.set(this);
    }); 

And considered all the other case – where we pass arbitrary strings – as generated by add-on, and therefore they are not belongs to any content document, at least for the time being. We should think how the private browsing mode could affect the add-ons.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 638109 [details]
Pull request 474

Ok, then, this patch definitely worth taking right now in order to fix failing tests on tinderbox.
Then, we can open a dedicated bug on proper private browsing handling in clipboard.set.

Comment 11

[Josh Matthews [:jdm]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #9)
> I'm still not quite sure about the document we could use when a developer
> want to set an arbitrary string generated from an add-on, that aren't
> related to any window or document.
> 
> I'd like also avoid for the developer to set an arbitrary document or window
> reference when he set some strings to the clipboard, as additional
> parameter. Instead, I will suggest to modify the `Clipboard.set` to accept
> also a `selection` instance (our jetpack selection instance), so that we can
> automatically detect the document / window it belongs. 
> An example of code could be:
> 
>     const clipboard = require("clipboard");
> 
>     require("selection").on("select", function(){
>        clipboard.set(this);
>     }); 
> 
> And considered all the other case – where we pass arbitrary strings – as
> generated by add-on, and therefore they are not belongs to any content
> document, at least for the time being. We should think how the private
> browsing mode could affect the add-ons.

For clipboard content that is not related to a document or window, null is fine. However, I really like your idea of taking selection objects; that is very logical and clean.

Comment 12

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/acc452b59219b5c9c57a90469960dfaf416b3e88
Bug 769440: Starting with FF16, nsITransferable have to be inited.

https://github.com/mozilla/addon-sdk/commit/4985566467c1aee779f4a3e2f3b3fb426cede77b
Merge pull request #474 from ochameau/bug/769440-fix-nsITranferable-init

fix Bug 769440: Starting with FF16, nsITransferable have to be inited. r=gozala

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c50b2ddc155

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 638109 [details]
Pull request 474

r+ in pull request.

Comment 15

[Naja Melan]

I didn't quite get whether this is fixed in firefox or just in Addon SDK? Eg. for addons not using the sdk, is it and will it stay necessary to call nsITransferable.init? I do get an exception when calling Services.clipboard.getData. If it isn't necessary, which firefox version contains the fix? 

If it is, I think the following pages on MDN should be updated: 
- Using the clipboard
- nsIClipboard

Note that there is no MDN page for nsITransferable. 

Thanks in advance

Comment 16

[Alexandre Poirot [:ochameau]]

It has been a long time since I looked at this, but looking at my comments, it looks like starting from FF16, we have to call init method. So yes MDN most likely need to be updated.
This fix only applies to SDK addons, regular XUL addons using nsITransferable manually have to be updated.
(SDK exposes an helper so that addons do not interact directly with nsITransferable)