595525 - Double data deletion in streamTest (nptest.cpp), test_streamNotify.html

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Honza Bambas (:mayhemer)]

After bug 537381 had been landed, test modules/plugin/test/test_streamNotify.html started to fail or timeout.

Problem is that we try to delete ndata (URLNotifyData*) in nptest.cpp twice during the test:
1. in MakeNewNPAPIStreamInternal [1] listener (nsNPAPIPluginStreamListener), holding reference to notifyData, is released and deletes the notify data in its destructor because we early exit on nsnpapiplugin.cpp:586 as we fail to get URL ("http://localhost:-8/", failure is correct with bug 537381)
2. in streamTest [2], when we get an error from call to either NPN_PostURLNotify or NPN_GetURLNotify, we delete the data again (delete ndata)

The timeout on windows is caused by assertion failure dialog box pop-up.

I don't understand the test so well to say what should the result of the particular test be with this URL.  But as I can see, before bug 537381 landed, this test was failing to load that invalid URL as expected, but later in the phase, not that early during creation of the URL.

[1] http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsNPAPIPlugin.cpp#558
[2] http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/testplugin/nptest.cpp#2328

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That stream stuff is not me, maybe bsmedberg?

Comment 2

[Benjamin Smedberg]

The testplugin is correct, and this indicates a bug somewhere in the plugin host or networking code. 

* If NPN_GetURLNotify returns a failure code, it must *not* call NPP_URLNotify.
* If NPN_GetURLNotify returns a success code, it *must* call NPP_URLNotify.

We added this test because this was broken once before and we fixed it as part of the OOPP work. Please fix it again!

I don't think you're supposed to call NPP_URLNotify at all until you've returned from NPN_GetURLNotify.

Did you back out the original patch, or is the tree currently broken?

Comment 3

[Honza Bambas (:mayhemer)]

Any chance to get this fixed or can we disable the test for a time?  This is blocking a final+ bug, so no immediate rush but the patch should bake in beta for a time as it might bring problems also in the outside world.

Comment 4

[Benjamin Smedberg]

Who are you asking? I figured you would take it. Nobody else has time, I think.

Comment 5

[Benjamin Smedberg]

And no, we cannot disable the test because it is testing something which actually happens in the wild and will cause that crash, which is a potential sg:crit.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to comment #4)
> Who are you asking? I figured you would take it. Nobody else has time, I think.

It really wasn't clear from your comment.  I have absolutely no knowledge of that code and I don't have much time either, however I can try follow your suggestions.  Can I ask you for review/feedback then?

Comment 7

[Benjamin Smedberg]

Yes, and/or Josh who is the module owner.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #5)
> And no, we cannot disable the test because it is testing something which
> actually happens in the wild and will cause that crash, which is a potential
> sg:crit.

We apparently can:

83 /*
84  * XXX/bsmedberg: disable this test as well, since the pluginhost is broken
85  * and the testplugin is double-deleting the urlnotify context.

So, this bug is actually a known problem, as I can see...  http://hg.mozilla.org/mozilla-central/rev/be2c775bddab

I might have a fix for this, but the test gets stuck.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: wip1

For consideration

Comment 10

[Honza Bambas (:mayhemer)]

Benjamin, for bug 537381 to get landed finally, would you be ok to disable the test for -8 port until we solve this?

Comment 11

[Benjamin Smedberg]

No! The host:-8 version works currently, and I'm sure that I added that check to make sure that a security bug remained fixed.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1

For review.
- changes to the host are self explanatory, also see the bug description
- test has been changed to expect a sync-failure ; therefor drop in the number of pending tests

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1

qref...

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 480105 [details] [diff] [review]
v1

With patch for bug 537381 passed try server.

Comment 15

[Josh Aas]

Comment on attachment 480105 [details] [diff] [review]
v1

"npAPIPluginStreamListener" - I'm not a fan of this variable name. It looks too much like a misspelled class name. Otherwise this looks good, thanks.

Comment 16

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [Check in comment 18]

Thanks for review.  Renamed to PluginStreamListener.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 482261 [details] [diff] [review]
v1.1 [Check in comment 18]

This is blocking a blocking2.0:final+ bug.  That bug should be landed in beta first as similar change already caused some trouble in the past.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 482261 [details] [diff] [review]
v1.1 [Check in comment 18]

http://hg.mozilla.org/mozilla-central/rev/c5afefc3d6f5