Opened 4 years ago

Closed 4 years ago

#99 closed defect (fixed)

Crash after destroying plugin instance

Reported by: dmik Owned by:
Priority: major Milestone: 0.4.2
Component: wrapper Version: 0.4.1
Severity: medium Keywords:
Cc:

Description

When working on Firefox 17 (https://github.com/bitwiseworks/mozilla-os2/issues/40), I found out that the plugin container process crashes after it calls the NPP_Destroy plugin callback to destroy the plugin instance (i.e. when the page containing Flash gets changed/reloaded/closed etc).

Debugging shows that it happens because the browser (plugin-container) calls static methods of NPObjects created on behalf of the plugin instance after calling NPP_Destroy to make sure all objects are properly deleted. In particular, it calls the invalidate method passing it the object to be deleted as an argument.

However, the Flash plugin on OS/2 is just a wrapper around the Win32 plugin. It creates wrapping stubs for all callbacks/functions/methods, including class methods of NPObjects. And in case of class methods these wrappers are bound to the plugin instance structure. And since this structure gets freed in NPP_Destroy, stubs are also freed. As a result, the browser tries to call arbitrary memory which leads to unpredicted behavior (SIGSEGV, SIGILL are common examples).

Change History (8)

comment:1 Changed 4 years ago by dmik

The reason why it didn't crash that often on FF10 is perhaps because in case of in-process plugins this memory would get reused much more rarely than in case of plugin-contanier. I would also guess that at least some Flash crashes under FF10 could actually be caused by this defect too.

comment:2 Changed 4 years ago by dmik

In fact, classes have nothing to do with instances, they are quite independent entities (regarding their lifetime in the plugin module). I'm pretty sure that in the real plugin class methods are static functions within the DLL (which are therefore always available).

So, we have to redo this part and add a separate map of Win32 -> OS/2 classes which is bound to the life time of the plugin module as a whole (i.e. guaranteed to be available within the NP_Initialize/NP_Shutdown scope).

comment:3 Changed 4 years ago by dmik

  • Milestone changed from Next to 0.4.2

comment:4 Changed 4 years ago by dmik

While working on this, I discovered some memory leaks, see #100.

comment:5 Changed 4 years ago by dmik

Btw, the crashes definitely happen with Firefox 10 as well. As I see now, at least #88 is the result of this problem. We should ask to re-check other crashing tickets once this defect is fixed.

comment:6 Changed 4 years ago by dmik

I also recall that I frequently had FF crashes when closing it after using Flash. This is also the result of this defect.

comment:7 Changed 4 years ago by dmik

I have completely rewritten NPClass wrappers code, see r121. Now it's not bound to the plugin instance and we may have any number of wrappers (instead of just one as before). I haven't seen it used in practice so far but it may be the case in future versions of the Win32 plugin or on particular sites, who knows. The new code is also simpler and cleaner.

The crash after destroying the plugin instance has gone now.

Note that I also found (and fixed) a nasty typo in the class wrappers: when browser called the removeProperty class function, it would be redirected to the hasProperty function within the Win32 plugin. I don't know how much this typo could influence the program logic and I'm not sure if we had any real problems because of that but that was clearly wrong and in theory you may imagine scenarios where it would be really bad. Now it's fixed too.

comment:8 Changed 4 years ago by dmik

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.