|IRC Nick||Time (GMT-7)||Message|
|jack_thompson||08:09||hmm... i have a plugin that compiles fine in linux and windows... but about:plugins of frefox/windows doesn't show it...
i renamed it to npudps32.dll
ad put it in the right place.. c:\program files\mozilla ...\plugins\
|taxilian||09:09||jack_thompson: you still around?|
|amackera||09:09||Evidently not :P|
|taxilian||09:09||guess not =]
he'll come back eventually
so amackera, when are you going to get that merge done for me? =]
I'll do it right now, if I get caught up in my stuff I'll never get a chance
|taxilian||09:09||hehe. I appreciate it|
|amackera||09:09||It's the taxilian-misc clone? or trunk?|
|taxilian||09:09||I'll then merge it into trunk, and you won't have to do it again
|taxilian||09:09||taxilian-misc has been merged into trunk already|
|taxilian||09:09||today if I can I will pull in the security zones stuff|
|amackera||09:09||So just to be clear the plan is for me to merge into trunk and commit?|
|taxilian||09:09||merge from trunk into your branch
I'll pull it into trunk
|amackera||09:09||Ok I see|
|taxilian||09:09||that way a second pair of eyes will go over the merge before it goes into head
I'd love to have that always be the standard
but usually there is nobody available to review my code when I push =]
|amackera||09:09||Haha yeah I hear you
I want so badly to have regular code reviews at work, but nobody apart from me has any C/C++ experience
Or time, for that matter, heh
|kalev||09:09||yeah, I would love to have someone go over my code too, but nobody ever does|
|taxilian||09:09||Georg used to go over anything I committed, and I pretty much review anything that goes into FireBreath, but everyone is so busy, it's hard to get a good code review anymore
it would also be interesting sometime to do some real heavy testing with Valgrind to see if there are any memory leaks in FireBreath
|kalev||09:09||oh, that reminded me something
not sure if you know, but I work with Antti Andreimann. He contributed a few fixes to firebreath some time ago.
he hasn't worked on the plugin project for a while, but last time he did he came up with a (in my opinon) incredible hack to work around a bug somewhere in NPAPI retain/release
I don't really know much about NPAPI
we were seeing crashes both in Windows and Linux with plugin unloading
and this patch makes the crashes disappear at least
what's interesting though is that the unloading crashes didn't happen with Firefox's new out of process plugin implementation; only when the plugin was running in browser's process
|amackera||09:09||Probably was fixed as a side-effect of hosting the plugin's own process|
|amackera||09:09||Seems like it's a bug in RetainObject(), not actually retaining objects :P|
|amackera||10:09||kalev: that's a neat hack/workaround
taxilian: I've merged trunk into my clone
|taxilian||10:09||kalev: sorry, I was away for a bit. Let me look over Antti's hack; yeah, I remember him
jack_thompson: welcome back =]
jack_thompson: this plugin that you're having problems with; is it a firebreath plugin or a straight npapi plugin?
kalev: for that doubleretain thing, can you tell me which browsers that has been seen on?
|kalev||10:09||taxilian: hold on, I'll call Antti and ask him if he remembers; Firefox was one of them for sure but I think there were more|
|taxilian||10:09||I know there are a couple of issues on this
but I want to make sure I am clear on where they stand
kalev: If there is any way that antti could pop in for a second, I have a couple of other questions.
for example, I don't understand why he would check for referenceCount < 1; if NPAPI is implemented correctly by the browser, ReleaseObject will automatically free the object if the reference count is < 1; so the second check should be at best redundant, at worst could cause undefined behavior (since the memory it is checking should be freed)
|jack_thompson||10:09||taxilian still npapi...
i am fighting my way through the vc++ howto....
|taxilian||10:09||jack_thompson: ok; I'll omit the obvious suggestion about "in firebreath this always works", then ;-) what crash are you experiencing?|
|jack_thompson||10:09||i should have tried that before i "cleaned it up"|
|taxilian||10:09||what version of visual studio are you using?|
|jack_thompson||10:09||oh... vc++6.0 xD
it the only vc++ i could get so quick
the plugin is detected and the page loads...
i'll try it with npruntime first...
i mean the howto...
to see if the old vc++ is the problem...
|kalev||10:09||taxilian: talked with Antti. He wasn't at computer right now but promised to come to this channel in an hour or maybe two.|
|taxilian||10:09||jack_thompson: well, that would definitely make it hard to use firebreath, then =] there is no reason you couldn't do npapi with that version
kalev: ok; I will probably be on then. I'm juggling school homework and work right now =]
|taxilian||10:09||jack_thompson: the plugin detected means you have your string resources correct; it crashes when the page loads?
oh, when you try to script it
sorry, didn't see the pm
make sure you're returning the object from NPP_GetValue correctly
it's not going to be a VC++ 6 issue
|amackera||10:09||Anybody have any good recommendations for intro to TDD or unit testing|
|taxilian||10:09||hmm. not here =] you could look over how the ones in FireBreath are written
you could even expand them for practice, if you want!
I'll be back in a bit; gotta move. (currently sitting on a bench in a random hallway)
|jack_thompson||10:09||aparrently vc++ 6.0 seems to be the problem... :o
but i dont see myself reinstalling windows on a larger virtualbox image... and then finding visualc net something..
i just ignore windows :D
|taxilian||10:09||jack_thompson: why do you say that vc++ 6.0 is the problem?|
|jack_thompson||10:09||because the npruntime sample doesn't work either...
it doesn't quit firefox.. but it freezes...
i dont know how else to verify i can build plugins in windows
i'll just release the plugin for linux and postpone the windows port infinitely... i will not allow win32 to get into my way of doing this
maybe someone else fixes it when i add the project files
|taxilian||11:09||if you use firebreath, I could probably take a look at it when you have the core working =]
(yes, I'm trying to get more open source examples to use firebreath)
the hardest part of getting a new plugin working with FireBreath is getting python working to generate the skeleton project and getting cmake installed
|jack_thompson||11:09||do plugins that use firebreath have python as dependency later? or is that just firebreath?|
that's just used to generate the initial project
you could do it by hand if you want
just faster to use python
(it uses the cheetah template engine to process the example files, generate your guids for activex, set your mimetypes, etc)
|taxilian||11:09||yeah, I wouldn't go for a python dependency either =]
here is the template it uses: http://code.google.com/p/firebreath/source/browse/#hg/fbgen/src
|jack_thompson||11:09||and i get a shared library... people dont need to install firebreath to use the plugin right?|
firebreath is a framework
i'll give it a try... my head is empty after clicking on so many stupid windows buttons... i'll watch some tv or so...
could be worse
I'm doing binary math on paper
i'll bother you again when i run into problems :D
if I'm not here, the other guys can most likely help you as well
|taxilian||11:09||oh, and a general FYI; I am always signed on, even if I'm not actually connected; I'll see any messages when I get back. That's why sometimes it takes me so long to answer|
|amackera||12:09||brb, stupid apple updates require restart, wtf!?|
|kalev||12:09||taxilian: talked to Antti again on the phone and he sounded like he might not get to computer any time soon
maybe tomorrow then.
|taxilian||12:09||kalev: ok; I'd be interested to know more about his findings, but I'll look at it. I might do something similar in FireBreath to protect against that edge case; it seems unlikely that it could cause problems, with the exception of the innermost if statement that seems a little strange|
|amackera||12:09||Oh knows taxilian, after merging to trunk all my terrible, terrible hacks don't work!
|taxilian||13:09||amackera: what terrible hacks don't work?|
|amackera||13:09||CAOpenGLLayer doesnt work in Safari anymore for some reason
I think i had a hacked prepmac.sh that i reverted
aha! but i made a backup beforehand!
|kalev||13:09||mercurial makes backups automatically if you revert something|
|taxilian||13:09||… and you can always go look in the history =]|
|kalev||13:09||no. in git yes, revert reverts already commited changesets and you can look at the history, but in mercurial revert just undoes changes to the working tree.
mercurial revert <file> == git checkout -- <file>
|taxilian||13:09||are you sure? I could swear I've used revert and it just pulled out the old version
|kalev||13:09||oh could be, maybe it does two different things|
|taxilian||13:09||sounds like he's got it covered|
|kalev||13:09||I'm no mercurial expert :)|
|taxilian||13:09||yeah, me neither
still debating if it's worth the trouble of moving firebreath to github
|kalev||13:09||your call. I personally prefer git over mercurial, but it's also nice to keep both version control and bug tracking / wiki at one place. Furthermore, it's probably extra work for people who have checkout out firebreath mercurial repo. Not sure how many there are, perhaps most people use either nightly snapshots or the released tarballs.|
that's the question
in a lot of ways, git would be much easier
and I could keep the wiki and the files where they are
|amackera||13:09||Turns out the breakage to my awful no-good hacks was entirely my fault (honestly was there any doubt?)|
well, as long as you can fix them
I was notified by Kalev that there is an interest in my "double - trouble - retain - or - die" hack ;)
I am not sure the hack is a good thing, othervise I would of pushed it to the source tree.
However applying it does reveal a number of deficiencies in Firefoxs' NPAPI.
The problems are only visible when event handlers are used.
When destroying the list that holds the event handler JSObjects, some objects show invalid reference counts, and the browser sometimes crashes.
BTW there is an if that checks if reference count is zero in Firefox code and refuses to free if so ;) It's an ugly hack that valgrind grunts about and it seems to me that reference counting is not very stable in FF.
We have seen crashes when unloading the plugin object, even with doubleretain patch applied.
I personally have witnessed an occasional crash in FF, our Mac testers told me that Safari crashes even more often.
No crashes with new FF out-of-process code.
I have some working theories about the crashes, but I haven't had any time to verify them :(
The plugin also crashes on unload if there is some other JSO retained in the JSAPI code. Eg. a plugin allows one to set a callback function.
We used to do that in our plugin.
I have a feeling that Firefox and Safari destroy JS objects in another thread at the same time as the main thread destroys the plugin and thus there is a race condition. The crashes usually show a value correspondig to a signed "-1" as the referenceCount (the variable itself is an unsigned int).
|taxilian||14:09||anttix: I am back now =]
reading your messages
|anttix||14:09||My doubleretain hack shows no errors with chromium or arora, but it will show that reference counts are off when running in FF.|
|taxilian||14:09||anttix: were you using multiple threads anywhere?|
I have even managed to crash the FBTestPlugin :P
|anttix||14:09||You need to add event handlers in test HTML for it to happen.
never on windows or mac?
|anttix||14:09||The crashes with our plugin have been visible on windows FF and Mac Safari too.
I did not test FBTestPlugin on windows
|taxilian||14:09||hmm. I wonder if we could be doing something wrong there|
|anttix||14:09||The reproducibility is quite bad. I get one crash in 7-10 loads/unloads.|
|taxilian||14:09||hmm. too bad I didn't have time to work with you on it when you were playing with it|
|anttix||14:09||And it usually requires me to restart the browser to try again, eg. it seems to break more easily when it's memory is fresh.|
so I'm looking at http://fpaste.org/Ypw6/
|anttix||14:09||It seems to me that reproducibility is a bit better with Safari but I do not have a Mac to play with right now.|
|taxilian||14:09||on line 28, why do you have that check there?
|anttix||14:09||Actually I have never hit that test.|
|taxilian||14:09||you may have
you might not know
it would crash
|anttix||14:09||It was there just in case to detect race conditions, but I haven't catched one
Well debugger would of shown me that then.
|taxilian||14:09||when you call ReleaseObject, if it goes to 0, it will delete the object
race conditions *should* never happen, because this *should* never be called on another thread
so I'm glad to hear you haven't hit it =]
|anttix||14:09||And reading the same memory afterwards will only utter a warning under valgrind :P|
|anttix||14:09||FYI the Firefox code is full of checks like that. Valgrind will grunt about reading freed memory all the time :P|
|taxilian||14:09||yeah, I know
that's the next thing I was thinking it would be interesting to try, if I can reproduce the issue
build firefox from source
and log the retains / frees
|anttix||14:09||Well, it might shed some light to the issue.|
this feels like it should be a plugin issue
since you'd think that if other plugins had this issue, it would have come up, y'know?
|anttix||14:09||At first I think You should test if You can get the same "buggy browser detected" warnings from my patch as I will get.
Those are 100% reproducible with my setup.
Crashes are not that common. That's why I think there must be a race somewhere.
|taxilian||14:09||ok; I'll have to play with it. it's easy to test on mac, not so easy on windows|
|anttix||14:09||Yeah, just make sure You register some event handlers from the test page.|
|taxilian||14:09||ok; it might be next week before I get to that|
|anttix||14:09||It only happens when plugin retains some JS objects.|
|taxilian||14:09||but I appreciate the info|
|anttix||14:09||I wish I had something more conclusive :(|
|taxilian||14:09||yeah. very strangeish
that's okay, it's more than I had
|anttix||14:09||Right now all I have is a bunch of hunches and some controversial test results.|
|taxilian||14:09||I've been unable to do any real work on firebreath for 6 months; it's good to have some help on some of the problems found in that time =]|
|anttix||14:09||Firefox reference counting will be off when using event handlers, that I can tell for sure.
But it shouldn't cause a crash.
|taxilian||14:09||well, that's definitely better than crashing|
|anttix||14:09||Because there is if(referenceCount > 0) free.... in FF code.|
|taxilian||14:09||so I'll probably at least put some of this in there
|anttix||14:09||The firefox releaseobject actually does this check.|
|taxilian||14:09||it's been awhile since I dug through that|
|anttix||14:09||I looked at the code, I don't remember the file name from the top of my head.|
|taxilian||14:09||I'll definitely need to look at that; if we find that this is the only way to make it reliable, we'll put it into firebreath
reliability is the key thing
more important than smallish leaks
|anttix||14:09||yeah, but ....|
|anttix||14:09||My patch didn't make the crashes disappear :(
because sometimes referenceCount will hit -1 :(
|taxilian||14:09||so you ran firebreath through valgrind a fair bit, huh? does it seem to be pretty good leak-wise?|
|anttix||14:09||But since it's unsigned integer, it will overflow.
Pretty good, yeah ;)
|taxilian||14:09||well, I'm tempted to just call retain 10 times to be safe :-/
|anttix||14:09||I was quite happy. The leaks we detected were mainly from openssl and stuff like that.|
|taxilian||14:09||considering that this is my 3rd real c++ project that I built from scratch, and the 2nd one had a lot of leaks in it, that's not bad
|taxilian||14:09||the 2nd one was my first complete plugin framework
firebreath is better
|anttix||14:09||FB is cool. I like it :) Othervise we would of invented our own cross-browser IDL compiler :P|
|taxilian||14:09||I'm glad you like it =] it will be getting an infusion of new features in the next bit as well
because Facebook has hired me to build them a plugin =]
|taxilian||14:09||that's where the UTF8 conversion stuff I just put in came from, actually|
|anttix||14:09||As soon as You don't provide any event handlers it will be rock-stable ;)|
|anttix||14:09||soon = long :P|
|taxilian||14:09||yeah, I got that =]|
|anttix||14:09||anyways, I wish You good luck in tracing that madness.|
|taxilian||14:09||thanks. I appreciate your input|
|anttix||14:09||I might lend You a hand if I get some time, but right now I'm quite busy.
We worked around the issues in FF by running our plugin in this new out-of-process container.
References are still off a bit, but no crashes so far.
My running theory is that firefox starts to destroy JS functions in one thread and plugins in the main thread.
firefox has checks in debug mode everywhere for thread id
throws a warning if anything calls retain or release from a different thread
the thing that makes me doubt this whole thing, though
|anttix||14:09||Well, for some ugly reason the functions passed as event handlers to FB code are the main sources of this problem.|
|taxilian||14:09||is that if the reference count is that far off
why doesn't it crash when we're not involved?
|anttix||14:09||I don't know :(|
|taxilian||14:09||I'm thinking there must be something else going on :-/|
|anttix||14:09||Anyway, if You have trouble reproducing the problems with reference counts, let me know, I'll do some more tests with my own setup.
Yeah, I was thinking about writing a small plain NPAPI plugin to test the issue, but didn't get too far with the idea.
|taxilian||14:09||I have heard that there are lots of ways to mess up an AutoPtr class; I wonder if there are problems with FireBreath's
maybe it's time to look at changing to the boost reference counting ptrs
to eliminate the possibilites
|anttix||14:09||That isn't such a bad idea after all ;)
But why are reference counts OK with chromium?
|taxilian||14:09||this is why people hate browser plugins =]|
|anttix||14:09||Yep. And this is why a framework that will deal with all the braindead NPAPI implementations out there is a darn good idea in the first place ;)
ok, I gotta go now. It's 11:53PM around here :P
thanks for popping in
if you find time, you might find JUCE interesting to look at
it's the closest thing to a competitor for FireBreath that I've seen
|anttix||14:09||I'll check it out. Thanx.|
|taxilian||14:09||might have some things we can learn from them|
|taxilian||14:09||I thought you left?
|anttix||14:09||Yeah I did ;) My pidgin just reopened the window. Ok, bye for the second time ;)|
|amackera||15:09||I tried using pidgin for a bit for IRC, pretty much fails as an IRC client|
|taxilian||15:09||I actually haven't had any problems with it
I'm using adium now
of course, I don't use many fancy irc features =]
hmm. so if I were to change to use the boost shared_ptr, I would not need an embedded reference count in the JSAPI class anymore
could this ever become a problem?
|amackera||15:09||Why would it, unless there's bugs in boost
are you hesitant to include boost as a dependency?
|kalev||15:09||shared_ptr is going to be included in the next C++ standard by the way|
|amackera||15:09||whenever that rolls out|
|kalev||15:09||it's already in tr1:: namespace in gcc
yeah, and it's going to take some more time before compilers start impementing all C++0x functionality
|taxilian||15:09||boost is already included
including shared_ptr.hpp, I believe
yeah, it's there
|kalev||15:09||it is, I've used it in our plgin|
|taxilian||15:09||I just didn't know how to use it at the time
that's a pretty far-reaching change, but I think it would help to make sure there are no problems with the reference counting
|amackera||15:09||Seems like a better solution than ref counting|
|taxilian||15:09||it uses atomic refcounts, which could be a good thing|
|kalev||15:09||allright, I'm off to bed too now, night!|
this shared_ptr change will not be backwards compatible for other plugins
I think it needs to be done
but it won't be backwards compatible
|amackera||15:09||For breaking changes my opinion generally is: do it, get it out of the way before more people start using the broken old way
if the change makes sense, which in this case it does
|taxilian||15:09||yeah, that's what I think too
actually, as I go through here, I can see a lot of possible places where the use of AutoPtr could be causing some problems
this definitely needs to be done
it will take me some time, though
maybe I'll put that in 1.2 as well =]
headed home; be back later
|amackera||18:09||taxilian: I'm getting what looks like a hang on Safari trying to GetArrayValues|
|taxilian||18:09||with the test plugin?|
|amackera||18:09||no on my plugin
i'll try with the tester
|taxilian||18:09||can you see if it is reproducable on the test plugin?
FBTestPlugin in trunk?
boy let me tell you… changing to shared_ptr is requiring a *lot* of small changes
but it's definitely what needs to happen
gotta run; be back in an hour or two
|amackera||19:09||ok sounds good
you're not going to like this taxilian... FBTestPlugin crashes on a clean build at FB::wstring_to_utf8()
Firefox 3.6.8, Mac OS X 10.6
Same crash in Safari for Mac OS X 10.6 (same machine)
I'll look into it
do you know where in the function?
and/or what the value is that was being converted?
|amackera||20:09||not sure, i didn't look too far into it
but it happens on page load
|taxilian||20:09||ok :-/ I'll look at it
just trying to finish this autoptr conversion in one hit =]
and I'm always much stronger at development on windows
because I have more experience there
|taxilian||20:09||Hmm. I wonder if I should change the FB::PluginEvent objects to use shared_ptr as well
would help prevent memory leaks, I'm thinking
but also makes things a little more complicated
and breaks backwards compatibility further
they are currently ref counted?
|taxilian||20:09||Actually; I don't think I'm normally actually allocating those on the heap
I think they're on the stack
so they should be good
|amackera||20:09||you're fine then
well i'm headed home for the night