IRC Log Viewer » #firebreath » 2010-11-09

IRC Nick Time (GMT-7) Message
iaincollins 07:11 Georg's JSAPIAuto stuff is just awesome
also, bonus points to whoever put the comment in the code that says (to the effect) "If you get a linking error that takes you here, this is what you did wrong" (when I did it wrong on purpose and used int instead of FB::variant just to see what happened)
taxilian 09:11 iaincollins: you're still on 1.2.2, aren't you :-P
that linking error is gone in 1.3.0
iaincollins 09:11 *guilty shame* yes :(
I am a stupid amount of work to do so haven't been able to move across (as people are pushing for a demo, even am I'm being asked to work on other projects (boo))
but I am aware if I have a problem I'm not allowed to mention it until I'm on 1.3.x 8)
in fact I just ran into a related problem
and I'm ahead of where I expected to be today (due to FB being helpful), so I may move tomorrow
kalev 09:11 taxilian: hey, you were talking about cross thread marshalling and timer problems in Safari the other day; I can easily reproduce a crash in Firefox 4 in Linux if I fire an event in another thread.
are you interested in seeing the backtrace? Probably the same problem as in Safari.
taxilian 09:11 lol. moving from 1.2.2 to 1.3.0 really isn't that hard
kalev: sure, let's see it
pastebin?
I doubt it's the safari issue, though, since that seems to be specific to scheduletimer, which is a mac-only function
hmm. I have narrowed down the memory leak to async calls, and specifically passing a JSAPIPtr as one of the arguments
kalev 09:11 here: http://fpaste.org/6qRX/
it's thread 2 crashing, other threads don't seem to be doing anything relevant, but I still included their backtraces.
taxilian 09:11 kalev: is this linux?
n/m, it is
hmm
kalev 09:11 works fine in Firefox 3.6
taxilian 09:11 hmm. very interesting. it looks like pluginthreadasynccall isn't provided, so it's using the mac workaround with scheduletimer
they must have implemented it in firefox 4 on linux
but this looks like it's ScheduleTimer (a npapi function) that is crashing
this looks very much like a firefox bug
kalev 09:11 grr.
taxilian 09:11 without being able to reproduce it myself I don't know how else to verify what is going on
if you want to do some debugging, I can tell you where to look
kalev 09:11 Sure, I can give it a try
taxilian 09:11 ok, hang on and let me find the places you need to look
are you on dev or stable?
kalev 09:11 stable
taxilian 09:11 http://code.google.com/p/firebreath/source/browse/src/NpapiPlugin/NpapiTypes.cpp#122
this is where it decides if it should try to use pluginthreadasynccall or scheduletimer
aparently it is deciding to use scheduletimer
here is the function it uses instead of pluginthreadasynccall if it is using scheduletimer: http://code.google.com/p/firebreath/source/browse/src/NpapiPlugin/NpapiPluginModule_NPP.cpp#128
kalev 09:11 I guess an easy fix would be to use #if defined(XP_MACOSX) && defined(__LP64__) at http://code.google.com/p/firebreath/source/browse/src/NpapiPlugin/NpapiTypes.cpp#123
taxilian 09:11 depends
look closer at the code; it only overrides it if pluginthreadasynccall is NULL
in which case async calls aren't possible otherwise
so if firefox 4 drops support for that method, we need an alternatives
kalev 09:11 ah yes, there's some messed up indenting which had me confused
taxilian 09:11 ahh. sorry, I'll need to run the "convert all tabs to spaces" script again
kalev 09:11 OK, so I tried to change that line to what I said above and it made the crash go away
so pluginthreadasynccall obviously still works.
taxilian 09:11 ok; the check much be wrong for some reason
I would prefer to fix the detection rather than add an ifdef
if possible
kalev 09:11 there is currently no detection happening: on 64 bit platforms currently the existing #ifdef __LP64__ always picks the first option
the if (!srcFuncs->pluginthreadasynccall is never executed for me, because this is in a different #ifdef block
taxilian 09:11 ahh
you're absolutely correct; I'm not paying attention properly
sorry, I'm in class :-P
kalev 09:11 so I just added mac check to the existing __LP64__ line to make sure it doesn't get used on linux
taxilian 09:11 right
that's perfect
I'm sorry
be back in a bit; class just got out, gotta move to another location
kalev 09:11 Okay, pushed
thanks for your help, I'm off now for a bit.
taxilian 10:11 kalev: thank you for the fix
anyone here know how virtual destructors work well enough to confirm a theory for me?
neilg_ 10:11 taxilian: Possibly
taxilian 10:11 so if I have an object that inherits from another
in order for everything to get cleared up, I need the base class to have a virtual destructor, correct?
neilg_ 10:11 Yes
taxilian 10:11 what happens if the child class doesnt' have a destructor?
neilg_ 10:11 It does, one is automatically created for you if you don't specify one
So the parent's destructor won't be called in any case. At least there's no guarantees made!
taxilian 10:11 hmm. are you absolutely certain?
see, here is what has been happening
neilg_ 10:11 It really depends on the pointer to the instance you have though as to what happens
If the pointer is of the parent type then it will call the correct destructor
taxilian 10:11 somehow the Functor object stored for an async call hasn't been getting destroyed
but the object containing it should definitely be getting deleted, since it's in an auto_ptr
but deleting an object when you have a pointer to the base class definitely *can* call the child class's destructor as well
but I didn't have a destructor
in the child class
but the base class did and it is virtual
however, after adding a destructor to the child class, it seems to be working; trying to verify that this is logical =]
neilg_ 10:11 That makes perfect sense
taxilian 10:11 ok =]
well, good; this fixes a more serious memory leak as well, then
neilg_ 10:11 I really should have asked what the pointer type was in the first place ;) But I find it very good practice to always make my destructors virtual. It costs me nothing (okay, I may now have a virtual table that I didn't before!) and helps prevent problems like that
taxilian 10:11 right; I usually do as well. my destructor was virtual, but I didn't realize that my childclass had to have a destructor as well for it to work correctly. it does kinda make sense, though
ahh, well, step by step we learn
neilg_ 10:11 Of course, we totally do. Besides, C++ is completely weird and so, so complicated
If the subclass doesn't have a destructor then though one is automatically created for it, it won't be added to the virtual table. It's only there so that the delete operator works - I believe that's its only purpose
That's just one of those weird bugs because conceptually it seems like it would just work...
taxilian 10:11 yeah; I guess the destructor would not be needed if I had a pointer to the child class instead of the base class
but since I don't, I have to have a destructor there for it to work correctly
it's a templated child class, so having a pointer to it directly is not reasonable
ahh, well; very good to have that bug fixed
it was worrying me
I think when I finish this round of bugfixes I will release a 1.3.1
neilg_ 10:11 Sure, I know how that is. I hate not knowing why something that I thought should work just... doesn't
But we all learn through trial and error :)
taxilian 10:11 yep.
grr. some headhunter keeps trying to contact me. I told him in email very clearly "I don't have time to talk to you right now. I'm too busy. Feel free to email me, but I don't have time to talk by phone."
and he keeps calling
neilg_ 10:11 That totally sucks. If it helps - it's even worse when they call you at work but call through your boss's phone. Yup, had that happen several times. It's awkward anyway but when your boss is right next to you? Ouch!
taxilian 10:11 yeah, I've had that happen a few times. I'm just like "do you realize that I'm at work?"
'course, I also had my manager recommend me to one once... never a good sign
neilg_ 10:11 Ouch! Okay, you win!
taxilian 10:11 =]
the company I was working for at the time is now considering using FireBreath
neilg_ 10:11 It's great to see so many people jumping on board!
taxilian 10:11 it is definitely exciting
we've seen relatively few regressions as well, which is good
this memory leak is probably the first I've seen in awhile, and it's not really a regression since it's a bug in a new feature
neilg_ 10:11 And that's always fantastic news :)
taxilian 10:11 yep
gives confidence to the framework
taxilian 11:11 hmm. while that has fixed the issue on firefox, the issue on IE remains :-/
which means there are actually two issues
neilg_ 11:11 Which IE issue is that? I guess windowless support?
taxilian 12:11 no, the reference counting issue
I think I may have tracked it down, though
because of weirdness, an extra addref happening on the activex object, which means it never gets released
yep, think it's fixed
excelent
this one is a bug that has been there pretty much since the dawn of time, it would seem
I never would have seen it if I weren't looking for it
iaincollins 12:11 FYI moved over to 1.3, all good
taxilian 12:11 see? I told you it wasn't that bad =]
iaincollins 12:11 :) changes actually make sense
taxilian 12:11 if you use the trunk, you'll even get the bugfixes that I'm working on now :-P
do we normally make changes that don't?
lol
iaincollins 12:11 (j/k)
taxilian 12:11 I'm actually pretty hard-nosed about what breaking changes I allow. I made amackera refactor his windowless support twice to avoid breaking changes :-P
iaincollins 12:11 ouch
taxilian 12:11 so if there is ever a change that doesn't make sense, ask me about it
iaincollins 12:11 I've still got a bug thats causing a crash in NPAPI browsers, I just need to go and read the docs I think though, I'm doing something bad I'm sure
taxilian 12:11 can you pastebin me the stack trace?
iaincollins 12:11 (I'm creating objects on the fly and I think I'm maybe not doing it right)
I'll check that out tomorrow and will if it doesn't look like it's me :)
taxilian 12:11 creating objects on the fly hsould be fine, but make sure you put them in the correct structure. if those are JSAPI object, use boost::make_shared and stick 'em straight in a boost::shared_ptr
are you using threading?
iaincollins 12:11 thanks, I'll do that, and no
taxilian 12:11 ok; that's the only bug I know of that could cause a crash
nobody but me seems to have noticed it, though
:-)
iaincollins 12:11 I can imagine :)
I have an array of instances of an object (in my case, devices on the network)
I am doing this:
FB::JSAPIPtr networkDevice = FB::JSAPIPtr(this->networkDevices[id]);
return networkDevice;
is that Bad and Wrong?
taxilian 12:11 what is the type of networkDevices[id]?
sorry
networkDevices
iaincollins 12:11 sorry yeah it's:
NetworkDevice **networkDevices;
networkDevices = new NetworkDevice *[numberOfDevices];
(simplified)
taxilian 12:11 and NetworkDevice derives from JSAPI?
or more likely JSAPIAuto?
iaincollins 12:11 Sorry yes, class NetworkDevice : public FB::JSAPIAuto
taxilian 12:11 then, yes, that is both bad and wrong
iaincollins 12:11 I wasn't sure if that was okay or not
taxilian 12:11 and likely the cause of your crash
iaincollins 12:11 figures :)
taxilian 12:11 however, if you were to change it to an array of boost::shared_ptr<NetworkDevice> objects, you'd be fine
iaincollins 12:11 ooh
Thanks :D
I was actually surprised it worked at all (and it only did in IE)
taxilian 12:11 'course, if it were me, I'd use a std::vector<boost::shared_ptr<NetworkDevicePtr>> (and typedef boost::shared_ptr<NetworkDevice> NetworkDevicePtr)
the reason it worked is because IE has a memory leak
so my fix today would have made it crash =]
what happens is when you put it in a shared_ptr, it starts refcounting it
when the refcount hits 0, it deletes it
... but you still have a poitner to it
this === bad
iaincollins 12:11 eep
taxilian 12:11 anything that lives in a shared pointer must always live inside a shared pointer
when you do that, you don't have to worry about memory (as long as you don't have a circular reference)
iaincollins 12:11 and yeah should be std::vector :)
taxilian 12:11 so it's very useful for the JSAPI stuff in particular, which doesn't go away until the browser is done with it
if you don't want a mutable array, you could also use boost::scoped_array
which basically you give an array to and it frees it when it goes out of scope automatically
just makes it less likely that you will make mistakes
hey, rob
iaincollins 12:11 I am actually pretty ignorant of boost, I really need to fix that
oh cool, thanks
physicsrob 12:11 hey -- how's it going
taxilian 12:11 I have been as well, until forced to start learning of late
pretty good; tracked down the memory leak keeping the JSAPI objects from getting released
physicsrob 12:11 fun
iaincollins 12:11 thanks again, was relucant to bother people about that as I'd not done my homework to see what I should be doing and figured it was badness on my part
physicsrob 12:11 I'm hating Apple more and more every day :)
taxilian 12:11 hmm. however, there doesn't seem to be anything I can do to make it call the destructor when the browser closes :-/ IE just doesn't notify us
lol
still unreliable on the timer thing, huh?
physicsrob 12:11 yeah
it sucks
taxilian 12:11 if you want to give me a summary of what you've found, I'll see if I have any suggestions for you
I'll brb
physicsrob 12:11 and I can't step through my code because I keep on getting "Previous frame inner to this frame (gdb could not unwind past this frame) "
which makes debugging impossible
taxilian 12:11 huh... this is on mac?
physicsrob 12:11 yeah
taxilian 12:11 weird
never seen that
did you try playing with Georg's code?
physicsrob 12:11 no not yet
yeah -- gdb is totally freaking out on me
I have no idea why
taxilian 12:11 are you using just GDB or xcode?
physicsrob 12:11 xcode
taxilian 12:11 hmm
no idea
still getting the crash, though?
physicsrob 12:11 yeah
so
my timer fix made FBTestPlugin work
but my plugin was getting some other timer related crash ( I think it was JSC::something or other)
if I put a setTimer(myoriginalonload, 0) in my onload everything was fine
taxilian 12:11 ... that's weird
physicsrob 12:11 also -- if I put an alert at the beginning of my onload everything was fine
taxilian 12:11 hmm. I guess we could actually call the first one that way
but I'm not sure if it would help or not
interesting
physicsrob 12:11 yeah -- I tried that
overrode setReady to evaluateJavaScript("setTimer(\"onloadfunc();\",0);")
which helped, but now I'm getting some other crash
which I can't find
because gdb doesn't work
:(
taxilian 12:11 hmm. there is actually a better way to do the setTimer bit, but nonetheless...
have you tried using logging to see what is going on?
physicsrob 12:11 no, I haven't played around with the logging yet
any advantage to using logging over printf's on mac?
taxilian 12:11 'course, what it really sounds like is that ScheduleTimer just isn't reliable
not if you're able to see the printf
I wasn't able to see them when the plugin was OOP
physicsrob 12:11 oh yeah, that's true
this isn't a problem with OOP
I'm debugging with safari on leopard
taxilian 12:11 ahh, right
remember now
physicsrob 12:11 but that's a good idea nonetheless
when all else fails -- printf debugging
by the way -- fprintf(stderr, ...) do get logged in snow leopard -- they just go to the console
taxilian 12:11 ahh; didn't think of trying stderr
physicsrob 12:11 /var/log something or other
taxilian 12:11 or just run "console"
excelent... I now have the destructors getting called even on browser close, despite the activex control not getting properly released by the browser
physicsrob 12:11 cool
that's a nice improvement
so what is this better way than calling evaluateJavaScript("setTimeout ...
?
taxilian 13:11 setTimeout is a method of window
so you can getDOMWindow and then call setTimeout directly
no eval required
physicsrob 13:11 cool
taxilian 13:11 in fact we should possibly update the evalJavascript to call window.eval instead; it might restrict you to the page's javascript security levels is the only thing
but it would allow us to get a return value, which we otherwise can't do on IE
I am generally opposed to using evalJavascript, however, so I haven't spent any time on this
physicsrob 13:11 I'm surprised by how powerful the javascript API is
it's awesome, though, don't get me wrong
taxilian 13:11 in FireBreath? the javascript API is the main thing that inspired the project
physicsrob 13:11 yeah of course, plugins would be basically useless without some sort of javascript API -- it's just a bit richer than I would have expected
taxilian 13:11 well, understand that I was a javascript developer when I started learning to do plugins
I got frustrated with how bad the API was
on our plugin
I figured it had to be possible to do it better
turns out it was =]
I'm tempted to write an Ajax library for FireBreath just using pure javascript calls :-P
physicsrob 13:11 heh
okay.. so I've isolated my crash and it doesn't appear to be firebreath related.. but I'm still baffled
I'm doing an std::string::compare of a string and a const char *
and before I do the compare I print the string out
and everything is fine.. yet the compare crashes
boo
woohoo -- nevermind -- I found it
okay.. now with this crash gone I'll be able to genuinely test the firebreath timer stuff
physicsrob_ 13:11 taxilian: I don't see setTimeout anywhere in firebreath -- can you elaborate?
taxilian 13:11 hang on
there is no current abstraction for it
though it would be easy to add
physicsrob_ 13:11 gotcha
taxilian 13:11 and you don't have to add one to use it
just makes it more convenient
and would be a cool addition, IMO
physicsrob_ 13:11 how would you use it?
taxilian 13:11 http://www.firebreath.org/display/documentation/class+FB+DOM+Window
host->getDOMWindow() will give you that object for the JS window object in your page
note that DOM::Window extends DOM::Node: http://www.firebreath.org/display/documentation/class%20FB%20DOM%20Node
physicsrob_ 13:11 Ah okay
taxilian 13:11 so you use callMethod<FB::variant>("setTimeout", FB::variant_list_of(method)(timeout));
it would be cool to add setTimeout and setInterval on the DOM::Window object itself
physicsrob_ 13:11 agreed
taxilian 13:11 particularly since if you have it accept a JSAPIPtr you should be able to give it a C callback using Georg's callback stuff
physicsrob_ 13:11 so do you have any thoughts on whether it would be a bad thing to change the implementation of setReady to use this?
taxilian 13:11 whcih I haven't learned how to use yet
well, I don't think it woudl be a problem to use that for setReady
however
in this case I think that masks the real bug
and thus doesn't solve the problem
physicsrob_ 13:11 agreed
in my case I believe the only thing asynchronous in my plugin is the onload
so it solves my problem -- but not everyone elses
taxilian 13:11 well
lots of things need the async stuff that you may not realize initially
events, for example
physicsrob_ 13:11 I'll be honest -- I didn't think asynchronous things (like events) would be bug free when I started implementing
so I avoided all of that stuff
but anyway
well... I'm going to dive back into my fix
taxilian 13:11 hehe. well, they are... except on Mac, aparently :-P
ok
physicsrob_ 13:11 maybe the fix I came up with really did solve the problem, and it was this other bug that made me question it
taxilian 13:11 that would be nice
physicsrob_ 13:11 nope
Thread 0 Crashed: 0 com.apple.WebKit 0x968580e5 PluginTimer::fired() + 53
hmm
taxilian 13:11 gotta run and eat lunch
bbl
hmm
good luck :-/
taxilian 14:11 physicsrob_: so what is PluginTimer::?
physicsrob_ 14:11 part of WebKit apparently
this crash is a little bit strange though -- in a simple set up everything works great
the problem is that in my webapp I have to move the plugin. I detach the plugin element from one element and attach it to another
that triggers the whole shutdown/startup process for the plugin
and everything used to work great for this, and everything DOES work great if I do setTimeout
but if I don't use setTimeout, and just use my timer fix, the browser crashes after the plugin is reattached
it looks like Safari crashes after completing calling onload the second time
taxilian 14:11 hmm. that's strange
well, I guess we're going to need to come up with our own timer functions
I thought you were the one who suggested one?
physicsrob_ 14:11 I was suggesting a more consistent abstraction
since, for example, PluginWindowCocoaCG has a timer abstraction implemented
which seems like a bad way of doing it
taxilian 14:11 yeah; Georg has been looking into doing a more permanant abstraction
physicsrob_ 14:11 I think you're right, though -- we probably shouldn't be using the browsers timer functions at all
taxilian 14:11 but before we can do that, we need to come up with a reliable way to do it on all platforms
physicsrob_ 14:11 well, Georg has it taken care of for cocoa, right?
I definitely could implement it for carbon
taxilian 14:11 I haven't looked at what he sent
I will help you if you want to try to figure that out; I have some time this afternoon, and I'm going to need all this working
physicsrob_ 14:11 unfortunately I'm really pressed by my own deadlines -- I don't have a lot of spare time.
taxilian 14:11 today I'm pretty much dedicating to bugfixes
physicsrob_ 14:11 but I would love to help
taxilian 14:11 sounds like you'll need it to get your stuff working anyway
physicsrob_ 14:11 yeah, but setTimer works :)
setTimeout rather
taxilian 14:11 well, okay
I would be a little nervous with that fix myself =]
but I understand
physicsrob_ 14:11 I want to help, though
hmm
taxilian 14:11 basically we just need a way to call something on the main thread with a single opaque pointer as a parameter
physicsrob_ 14:11 one sec
taxilian 14:11 in Carbon we could do that simply by checking a threadsafe queue any time we get a NULL event
physicsrob_ 14:11 right
taxilian 14:11 so if Georg has a reasonable solution for Cocoa, it should be pretty easy to use, i'd expect
physicsrob_ 14:11 brb
taxilian 15:11 I'm considering packaging bash with FireBreath so that we don't have to use batch files. what thinks the group?
it would increase the size of the .zip archive by almost a meg
taxilian 18:11 I hate dos batch files
just in case anyone wondered