#14261: wxPluginLibrary::RestoreClasses() throws an exeception

7 views
Skip to first unread message

wxTrac

unread,
May 1, 2012, 8:48:45 PM5/1/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261>

#14261: wxPluginLibrary::RestoreClasses() throws an exeception
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 1 | Blocking:
-----------------------------+----------------------------------------------
When wxPluginManager::UnloadLibrary() is called on more than one plugin
dll that link a common dll, the second and subsequent calls throw an
exception in wxPluginLibrary::RestoreClasses().

For example, envisage a host.exe that loads plugin1.dll and plugin2.dll
using wxPluginManager::LoadLibrary(). All common functionality is in
another dll file, common.dll.

All files are built as "DLL Debug" and link to a "DLL Debug" build of
wxWidgets.

Tracing through RestoreClasses() with a debugger shows that plugin1.dll is
unloaded without any issues but when plugin2.dll is being unloaded,
RestoreClasses() attempts to restore classes from common.dll and the
wxWidgets dlls that have already been restored when plugin1.dll was
unloaded.

I've attached a patch that fixes the issue.

All my projects are built on Windows using VC++ 9.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261>

wxTrac

unread,
May 2, 2012, 6:31:28 AM5/2/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:1>

#14261: wxPluginLibrary::RestoreClasses() throws an exeception
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: infoneeded_new
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 1 | Blocking:
-----------------------------+----------------------------------------------
Changes (by vadz):

* status: new => infoneeded_new


Comment:

I'm sorry, I don't know much about these classes so I don't quite
understand what is the problem here: which exception is being thrown? Does
the code crash? If so, where does it do it exactly?

I also unfortunately don't understand what the patch does. Adding checks
for `NULL` is surprising as if the `wxClassInfo` linked list really
contains any `NULL` pointers, there is already a huge problem, they're
never supposed to be added to it. Does it really happen in your case? If
so, for which class?

OTOH the check for `info->GetClassName() != end` is fine but shouldn't
actually change anything because calling `erase(end)` doesn't do anything
anyhow.

Thanks in advance for any extra information/explanations!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:1>

wxTrac

unread,
May 4, 2012, 12:21:49 AM5/4/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:2>

#14261: wxPluginLibrary::RestoreClasses() throws an exeception
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 1 | Blocking:
-----------------------------+----------------------------------------------
Changes (by manyleaves):

* status: infoneeded_new => new


Comment:

I've attached a minimal VC++ 9 solution that demonstrates the problem.

When you click File->Exit the first plugin unloads correctly but you get
an access violation for the second.

If you expand on the patch I sent as follows:

#define APPLY_TICKET_14261
void wxPluginLibrary::RestoreClasses()
{
// Check if there is a need to restore classes.
if (!ms_classes)
return;

for(const wxClassInfo *info = m_after; info != m_before; info =
info->GetNext())
{
#ifndef APPLY_TICKET_14261
ms_classes->erase(ms_classes->find(info->GetClassName()));
#else
if (info == NULL)
break; // place a breakpoint here
wxDLImports::iterator it = ms_classes->find(info->GetClassName());
if (it == ms_classes->end())
continue; // place a breakpoint here
ms_classes->erase(it);
#endif
}
}

and place breakpoints where suggested you should see the issue ...

Thanks


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:2>

wxTrac

unread,
May 4, 2012, 5:29:09 AM5/4/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:3>

#14261: wxPluginLibrary::RestoreClasses() throws an exeception
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 1 | Blocking:
-----------------------------+----------------------------------------------

Comment(by vadz):

I'll try to find time to debug this but it would be really great if you
could just explain what the problem is if you already debugged it. E.g. do
you know where does this `NULL` pointer come from?


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:3>

wxTrac

unread,
May 10, 2012, 3:29:27 AM5/10/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:4>

#14261: wxPluginLibrary::RestoreClasses() throws an exeception
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 1 | Blocking:
-----------------------------+----------------------------------------------

Comment(by manyleaves):

I've now had a chance to look into this a little more deeply.

The way UpdateClasses() and RestoreClasses() use m_before and m_after
enforce an undocumented requirement that wxPluginLibrary files be unloaded
strictly in the reverse order that they are loaded otherwise m_before may
become NULL because a previous m_after had unloaded the class.

ms_classes doesn't seem to be used for anything anywhere so the attached
patch simply #ifdef's the relevant code with
wxUSE_DYNAMIC_LOADER_MS_CLASSES. This leaves the original code in place in
case anyone out there has a need to use ms_classes.

wxPluginLibrary files can now be unloaded in any order.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:4>

wxTrac

unread,
May 15, 2012, 6:03:30 AM5/15/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:5>

#14261: wxPluginLibrary::RestoreClasses() throws an exception if unloaded in wrong
order
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: new
Priority: low | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 0 | Blocking:
-----------------------------+----------------------------------------------
Changes (by vadz):

* priority: normal => low
* patch: 1 => 0


Comment:

I think the whole point of using `wxPluginLibrary` is that it handles
`wxClassInfo` for the classes defined in the shared library. Otherwise why
would you use it at all? Perhaps you just need `wxDynamicLibrary`?

Also notice that apparently you shouldn't use it directly but only via
`wxPluginManager`. I don't know this code at all but perhaps the problem
doesn't happen in this case?

Anyhow, because of the first point I really don't think it makes sense to
apply this patch.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:5>

wxTrac

unread,
May 15, 2012, 8:30:23 PM5/15/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:6>

#14261: wxPluginLibrary::RestoreClasses() throws an exception if unloaded in wrong
order
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 0 | Blocking:
-----------------------------+----------------------------------------------
Changes (by manyleaves):

* priority: low => normal


Comment:

If you've had time to take a quick look at the attachment -
ticket_14261.rar (which serves as a minimal example of the problem) you'll
see that I strictly use wxPluginManager and don't access wxPluginLibrary
directly.

wxDynamicLibrary isn't adequate for my requirements. wxPluginLibrary
automatically registers/unregisters any wxModule classes it finds in the
plugins it loads - that is why I'm using it. Also, I notice that the
static member wxPluginLibrary::ms_classes is populated (by
UpdateClasses()) and depopulated (by RestoreClasses()) but doesn't seem to
be accessed by any other code in wxWidgets - so why maintain ms_classes in
the first place?

Fixing this issue is quite important for me.

I am in the early stages of building a wxComposite framework for
wxWidgets. Inspired by Microsoft's Prism framework (see
http://compositewpf.codeplex.com/releases/view/14982), I hope to bring
similar functionality to wxWidgets.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:6>

wxTrac

unread,
May 16, 2012, 9:06:19 AM5/16/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:7>

#14261: wxPluginLibrary::RestoreClasses() throws an exception if unloaded in wrong
order
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: confirmed
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 0 | Blocking:
-----------------------------+----------------------------------------------
Changes (by vadz):

* status: new => confirmed


Comment:

Sorry, I didn't have time to look at the attachment. Understanding other
people code simply takes more time than I can afford, this is why I always
ask to explain the problem in a simple way or provide a minimal
[HowToSubmitPatches patch to a sample].

Anyhow, yes, if you need to register `wxModule`s dynamically, you must
indeed use `wxPluginLibrary` so it would be nice to fix this.
Unfortunately I'm afraid it's not as simple as your patch.

`wxPluginLibrary::ms_classes` is used by `wxIMPLEMENT_PLUGGABLE_CLASS()`
in `wx/object.h`. To be honest I have no idea how does this work and there
is no documentation for these macros but it seems clear that removing
`ms_classes` would break this. So either these macros should be removed as
well -- if we consider that they're useless -- or `wxPluginLibrary` should
be fixed to work correctly independently of the unload order. If you can
look at this in more details and explain what do you think should be done,
it would be very helpful.

TIA!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:7>

wxTrac

unread,
May 16, 2012, 9:32:16 PM5/16/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:8>

#14261: wxPluginLibrary::RestoreClasses() throws an exception if unloaded in wrong
order
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: confirmed
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 0 | Blocking:
-----------------------------+----------------------------------------------

Comment(by manyleaves):

Thank you for your patience with this defect. I'm a seasoned developer but
relatively new to wxWidgets.

I've looked again and the actual defect is quite simple to fix properly.

m_before is initialised in the wxPluginLibrary constructor
m_before = wxClassInfo::GetFirst();
which points it to the last class loaded before the plugin is loaded; but
the logic within UpdateClasses() and RestoreClasses() really needs it to
point to the first class loaded by the plugin.

The attached patch performs the equivalent of
m_before = wxClassInfo::GetFirst()->GetPrevious();
and changes the for() loop exit conditions in UpdateClasses(),
RestoreClasses() and RegisterModules() to match.

Modules can now be unloaded in any order.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:8>

wxTrac

unread,
May 26, 2012, 8:18:10 AM5/26/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:9>

#14261: wxPluginLibrary::RestoreClasses() throws an exception if unloaded in wrong
order
-----------------------------+----------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: confirmed
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Keywords: wxPluginLibrary | Blockedby:
Patch: 0 | Blocking:
-----------------------------+----------------------------------------------

Comment(by vadz):

Thanks for the explanation, I do indeed see the problem now: if a plugin
loaded earlier (call it `A`) is unloaded before another plugin loaded
after it (call it `B`), then `B::m_before` becomes a dangling pointer
because it was the same as `A::m_after`, i.e. pointed to an object
destroyed when `A` is unloaded.

I have just two problems with the patch: first is that emulating
`GetPrevious()` is somewhat inefficient. But I don't think there is a
better way to do it so we'll just have to live with it. Second problem is
that `m_before` and `m_after` don't describe the variables correctly any
more. I'll rename them as part of applying the patch, please retest it
later and let me know if I broke anything while doing it.

Thanks!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:9>

wxTrac

unread,
May 26, 2012, 8:29:55 AM5/26/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:10>

#14261: wxPluginLibrary::RestoreClasses() throws an exception if unloaded in wrong
order
-------------------------+--------------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: closed
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxPluginLibrary
Blockedby: | Patch: 0
Blocking: |
-------------------------+--------------------------------------------------
Changes (by VZ):

* status: confirmed => closed
* resolution: => fixed


Comment:

(In [71571]) Fix bug with unloading wxPluginLibrary objects in "wrong"
order.

wxPluginLibrary objects had to be unloaded in exactly the reverse order to
which they were loaded in. This was not documented and was a serious
limitation for any realistic use of plugins anyhow, so fix it and allow
unloading them in any order now.

Instead of keeping a pointer to the last wxClassInfo not created by this
plugin, now keep a pointer to the first wxClassInfo that was created by
it.
This makes the code slightly more complex but this pointer, unlike the old
one, remains valid even if another plugin was unloaded.

Closes #14261.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:10>

wxTrac

unread,
Nov 7, 2012, 8:09:29 PM11/7/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:11>

#14261: unloading wxPluginLibrary objects in "wrong" order
-------------------------+--------------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: reopened
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxPluginLibrary
Blockedby: | Patch: 1
Blocking: |
-------------------------+--------------------------------------------------
Changes (by manyleaves):

* status: closed => reopened
* resolution: fixed =>
* patch: 0 => 1


Comment:

I've now had a chance to retest this in the latest 2.9.4 and it seems you
have inadvertently switched the logic with m_ourFirst and m_ourLast


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:11>

wxTrac

unread,
Nov 7, 2012, 8:45:06 PM11/7/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:12>

#14261: unloading wxPluginLibrary objects in "wrong" order
-------------------------+--------------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: reopened
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxPluginLibrary
Blockedby: | Patch: 1
Blocking: |
-------------------------+--------------------------------------------------

Comment(by markdootson):

Replying to [comment:11 manyleaves]:
> I've now had a chance to retest this in the latest 2.9.4 and it seems
you have inadvertently switched the logic with m_ourFirst and m_ourLast

This was fixed in trunk by r72533 I think.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:12>

wxTrac

unread,
Nov 7, 2012, 9:51:48 PM11/7/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:13>

#14261: unloading wxPluginLibrary objects in "wrong" order
-------------------------+--------------------------------------------------
Reporter: manyleaves | Owner:
Type: defect | Status: closed
Priority: normal | Milestone:
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxPluginLibrary
Blockedby: | Patch: 1
Blocking: |
-------------------------+--------------------------------------------------
Changes (by manyleaves):

* status: reopened => closed
* resolution: => fixed


Comment:

Replying to [comment:12 markdootson]:
> Replying to [comment:11 manyleaves]:
> > I've now had a chance to retest this in the latest 2.9.4 and it seems
you have inadvertently switched the logic with m_ourFirst and m_ourLast
>
> This was fixed in trunk by r72533 I think.

I just updated to 72922 and I can confirm that it is now seems fixed.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14261#comment:13>
Reply all
Reply to author
Forward
0 new messages