1352559 - Remove NPN_NewStream and related machinery

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P2)

Description

[Benjamin Smedberg]

NPAPI had an API that lets plugins send a stream to a browser to load into a browser frame.

Flash doesn't use that API; we're not sure any major plugin used it! It's really grody from a security perspective. So we'd like to remove it!

Here's the details, hopefully enough for somebody to pick this bug up. I'll mentor!

The API surfaces to remove are:

NPN_NewStream
NPN_Write
NPN_DestroyStream

The code implementing these lives in the following places:

in nsNPAPIPlugin.h/cpp
* remove _newstream
* remove _write
* remove _destroystream
* in "sBrowserFuncs" those functions should be replaced with nullptr
* Remove nsNPAPIPlugin::RetainStream

In nsNPAPIPluginStreamListener.h/cpp:
* remove nsNPAPIStreamWrapper and supporting code
* remove nsPluginStreamToFile and supporting code

In nsNPAPIPluginInstance.h/cpp:
* remove nsNPAPIPluginInstance::NewStreamFromPlugin

Remove PPluginStream.ipdl and its references in PPluginInstance.ipdl

Remove PluginStreamParent.h/cpp and PluginStreamChild.h/cpp

In PluginModuleChild.cpp:
* remove _newstream, _write, and _destroystream
* replace those with null in sBrowserFuncs

In PluginInstanceParent.h/cpp and PluginInstanceChild.h/cpp:
* remove glue code which creates and destroys PluginStream objects

Remove code from the test plugin (nptest.cpp) which uses these three APIs.

Remove tests for these APIs. I *believe* that test_pluginstream_newstream.html is the only test, but I'm not 100% sure. Run the mochitests in dom/plugins/test to check.

Comment 1

[Benjamin Smedberg]

It looks like we can't remove nsNPAPIStreamWrapper in this bug; it will have to wait for bug 1352567

Comment 2

[smfurqan]

Hello Benjamin. I'm currently looking to contribute to my first open-source project and I'm hoping that I could assist with this bug with your mentoring. I'm currently working on linux and have firefox built. Would I be able to work on this, and if so, could you please give me a little more information as to where I should start? Your help is greatly appreciated.

Comment 3

[Benjamin Smedberg]

Replied offline.

Comment 4

[dan1bh]

Hi Benjamin can I be assigned to this? Or is there another mentored bug that I could work on?

Comment 5

[Josh Aas]

(In reply to smfurqan from comment #2)
> Hello Benjamin. I'm currently looking to contribute to my first open-source
> project and I'm hoping that I could assist with this bug with your
> mentoring. I'm currently working on linux and have firefox built. Would I be
> able to work on this, and if so, could you please give me a little more
> information as to where I should start? Your help is greatly appreciated.

The bug summary that Benjamin wrote has a lot of good information about where to start with this. It shows where all the code is (or at least, where to start looking for it), what to do with the NPAPI function table pointers...

You're going to want to get rid of those three NPAPI functions and all other code that is used only by them (their related IPC code, for example). Then you're going to want to make sure tests pass, likely getting rid of some tests for the code you removed.

What else do you want to know? You can email me at my bugzilla address with questions, might be easier than going back and forth on this bug.

Comment 6

[dan1bh]

Greetings Josh/Benjamin

I am working on this nut have a question: I am working on this step: - In nsNPAPIPluginStreamListener.h/cpp:
* remove nsNPAPIStreamWrapper and supporting code
* remove nsPluginStreamToFile and supporting code

You say remove nsNPAPIStreamWrapper and supporting code - do you mean remove all references to it in nsNPAPIPluginStreamListener.h ? there is a class nsNPAPIStreamWrapper shall I remove this? and should I click on find all references and remove all those references too? the class is referred to in other files e.g. BrowserStreamParent.cpp(30, 3): nsNPAPIStreamWrapper* wrapper =
BrowserStreamParent.cpp(31, 22): reinterpret_cast<nsNPAPIStreamWrapper*>(mStream->ndata);

I was just wondering as you've not mentioned that I need to make any changes to other files e.g. BrowserStreamParent.cpp

Thanks guys,

Daniel

Comment 7

[dan1bh]

Oh I forgot can you point me too the tests that I should run for this? Thanks again

Comment 8

[Benjamin Smedberg]

Please note my comment above about nsPluginStreamWrapper: "It looks like we can't remove nsNPAPIStreamWrapper in this bug; it will have to wait for bug 1352567".

I don't think you should need to touch BrowserStream* for this change. This change is all about the plugin-provided streams.

I already commented about the tests. See above. If you need help figuring out how to run the tests, please send me a private email.

Comment 9

[smfurqan]

Hello Benjamin. I sent you an email a few days ago asking you how I would submit what I have worked on for review. Would you please be able to give me some information regarding it please?

Comment 10

[dan1bh]

Hello all smfurqan it looks like we are working on the same bug! oh well no matter. I would be interested to see how you get on anyway and it doesn't matter who's patch is used eventually. It's all about collaboration anyway! I have a (partial) patch ready and I was also wondering if there are any notes on how to upload it for review?

Comment 11

[Benjamin Smedberg]

smfurqan and dan1bh, please see this document for uploading changes for review: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial

In case it's not clear, you need to `hg commit` your changes.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch has more documentation about good commit messages and other tips.

Comment 12

[dan1bh]

Gosh those instructions are very confusing. I have made a .diff patch and I have followed the steps as best I could. I ran $./mach mercurial-setup and then $hg push review and all seemed to be going well until I get this error:

$ hg push review
(third party extension firefoxtree requires version 3.8 or newer of Mercurial; disabling)
(third party extension reviewboard requires version 3.8 or newer of Mercurial; disabling)
pushing to https://reviewboard-hg.mozilla.org/autoreview
abort: remote error:

Pushing and pull review discovery repos is not allowed!

You are likely seeing this error because:

1) You do not have the appropriate Mercurial extension installed
2) The extension is out of date

This is a partial patch and there are certainly bits missing as I have explained in my emails but if we can use this patch as a starting point and I would really like to get things moving again on this.

Comment 13

[dan1bh]

Greeting again!
Hi Benjamin I have updated using "pip install -U mercurial" and it has updated to 3.8. And now I get:

$ hg push review
pushing to https://reviewboard-hg.mozilla.org/autoreview
searching for appropriate review repository
redirecting push to https://reviewboard-hg.mozilla.org/gecko
abort: you must set mozilla.ircnick in your hgrc config file to your IRC nickname in order to perform code reviews

Any ideas? Cheers! D

Comment 14

[dan1bh]

I have sorted out the above error and have committed the patch. Benjamin did you get it through ok? I just used " hg commit -m" command. I didn't get any confirmation message so I just assume it has gone through ok?

Comment 15

[dan1bh]

Attachment: Bug 1352559 - Delete API NPN_NewStream NPN_Write NPN_DestroyStream. In nsNPAPIPluginStreamListener.h/cpp:* remove nsNPAPIStreamWrapper and supporting code // blocked by other bug * remove nsPluginStreamToFile and supporting code //blocked. Remove PPluginS

Review commit: https://reviewboard.mozilla.org/r/143844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143844/

Comment 16

[Benjamin Smedberg]

Thanks dan! I've marked this bug as assigned to you and I'll have a look over this shortly.

Comment 17

[Benjamin Smedberg]

Comment on attachment 8872360 [details]
Bug 1352559 - Delete API NPN_NewStream NPN_Write NPN_DestroyStream. In nsNPAPIPluginStreamListener.h/cpp:* remove nsNPAPIStreamWrapper and supporting code // blocked by other bug * remove nsPluginStreamToFile and supporting code //blocked. Remove PPluginS

https://reviewboard.mozilla.org/r/143844/#review147928

Thank you, this is a good first start. There is one major issue (missing .ipdl changes in this patch) and a set of smaller formatting and logic issues to fix up. Please ping me if you have any questions!

::: dom/plugins/ipc/PluginInstanceChild.h
(Diff revision 1)
>                               const nsCString& headers) override;
>  
>      virtual bool
>      DeallocPBrowserStreamChild(PBrowserStreamChild* stream) override;
>  
> -    virtual PPluginStreamChild*

I think this patch is missing some .ipdl changes. It's correct to remove this code, but only if PPluginStream is also removed and PPluginInstance updated.

::: dom/plugins/ipc/PluginInstanceParent.cpp:1799
(Diff revision 1)
>          // The stream has already been deleted by other means.
>          // With async plugin init this could happen if async NPP_NewStream
>          // returns an error code.
>          return NPERR_NO_ERROR;
>      }
>      if (s->IsBrowserStream()) {

This should probably become a MOZ_ASSERT(s->IsBrowserStream()) since all streams should be browser streams at this point. Then remove the `if` and unindent the block here.

::: dom/plugins/ipc/PluginInstanceParent.cpp:1808
(Diff revision 1)
>              MOZ_CRASH("Mismatched plugin data");
>  
>          sp->NPP_DestroyStream(reason);
>          return NPERR_NO_ERROR;
>      }
> -    else {
> +   

Nit, trailing whitespace needs to go.

::: dom/plugins/ipc/PluginModuleChild.cpp:957
(Diff revision 1)
>      (NP_VERSION_MAJOR << 8) + NP_VERSION_MINOR,
>      mozilla::plugins::child::_geturl,
>      mozilla::plugins::child::_posturl,
>      mozilla::plugins::child::_requestread,
> -    mozilla::plugins::child::_newstream,
> -    mozilla::plugins::child::_write,
> +    nullptr,
> +	nullptr,

nit, no hard tabs in our code: must be spaces (and that will fix the indenting problem)

::: dom/plugins/test/testplugin/nptest.cpp:482
(Diff revision 1)
> -    if (err != NPERR_NO_ERROR) {
> -      instanceData->err << "NPN_DestroyStream returned " << err;
> -    }
> -  }
> -  else {
>      // Convert CRLF to LF, and escape most other non-alphanumeric chars.

It looks like you removed an `if` block here (correctly), but then you didn't unindent the remaining code. Please unindent. Also below you have a closing brace commented out: please remove that completely.

::: dom/plugins/test/testplugin/nptest.cpp:1389
(Diff revision 1)
>      memcpy(nd->data + nd->size, buffer, len);
>      nd->size = newsize;
>      return len;
>    }
>  
>    if (instanceData->closeStream) {

This whole block is no longer relevant because the test plugin can't early-terminate a stream. So you can probably remove all of this "closeStream" bits from nptest.cpp: here's where they were added:

https://hg.mozilla.org/mozilla-central/rev/4806e2c8554e

Comment 18

[dan1bh]

Comment on attachment 8872360 [details]
Bug 1352559 - Delete API NPN_NewStream NPN_Write NPN_DestroyStream. In nsNPAPIPluginStreamListener.h/cpp:* remove nsNPAPIStreamWrapper and supporting code // blocked by other bug * remove nsPluginStreamToFile and supporting code //blocked. Remove PPluginS

https://reviewboard.mozilla.org/r/143844/#review147928

> I think this patch is missing some .ipdl changes. It's correct to remove this code, but only if PPluginStream is also removed and PPluginInstance updated.

For some reason the solution explorer in VS cannot find any .ipdl files and therefore I cannot delete the PPluginStream.ipdl file or make changes to PPluginInstance.ipdl. Is there a way around this? For instance would I be able to manually edit these files outside of VS?

Comment 19

[dan1bh]

I've implemented these changes Ben but I've some questions for you - I have emailed them over today! thanks Dan

Comment 20

[Benjamin Smedberg]

Answered email, I hope that's enough!

Comment 21

[dan1bh]

Attachment: Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,

Review commit: https://reviewboard.mozilla.org/r/154042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154042/

Comment 22

[Benjamin Smedberg]

Comment on attachment 8883084 [details]
Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,

https://reviewboard.mozilla.org/r/154042/#review159574

Daniel, thank you for your hard work and patience with this!

There's one semi-serious thing to fix here (the incorrect MOZ_ASSERT in PluginInstanceParent.cpp) and the rest of the issues are very minor formatting/commit message issues.

I can fix these for you and push this on your behalf, or if you like you can fix these issues. Let me know which you prefer.

::: commit-message-b6b63:1
(Diff revision 1)
> +ipdl files updated

Before final push we need to update this commit message to use our standard form: "Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery, r?bsmedberg"

Normally you'd do that using `hg commit --amend` but I can help update that for checkin if necessary. I know you've been fighting hg pretty hard.

::: dom/plugins/ipc/PluginInstanceParent.cpp:1799
(Diff revision 1)
>          // The stream has already been deleted by other means.
>          // With async plugin init this could happen if async NPP_NewStream
>          // returns an error code.
>          return NPERR_NO_ERROR;
>      }
> -    if (s->IsBrowserStream()) {
> +	MOZ_ASSERT(s->IsBrowserStream()

Something is wrong here. This line is missing a closing parenthesis, which you've included down below on line 1807, so the MOZ_ASSERT now wraps an unrelated statement.

I suspect you're compiling the default --disable-debug mode, which means that all of this code is being compiled away and you won't see the compile errors of anything inside this block.

I think you just need to close this assertion properly and keep the block below. You can un-indent the block if you like.

::: dom/plugins/test/testplugin/nptest.cpp:481
(Diff revision 1)
>    else {
>      outbuf.append("Error: no data in buffer");
>    }
>  
> -
>  // Convert CRLF to LF, and escape most other non-alphanumeric chars.

The original indenting of this block looks correct. I suspect you accidentally changed this indenting?

::: dom/plugins/test/testplugin/nptest.cpp:1382
(Diff revision 1)
>      nd->data = (char*) realloc(nd->data, newsize);
>      memcpy(nd->data + nd->size, buffer, len);
>      nd->size = newsize;
>      return len;
>    }
> -
> +  

nit, trailing space

Comment 23

[dan1bh]

Comment on attachment 8883084 [details]
Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,

https://reviewboard.mozilla.org/r/154042/#review159574

> Something is wrong here. This line is missing a closing parenthesis, which you've included down below on line 1807, so the MOZ_ASSERT now wraps an unrelated statement.
> 
> I suspect you're compiling the default --disable-debug mode, which means that all of this code is being compiled away and you won't see the compile errors of anything inside this block.
> 
> I think you just need to close this assertion properly and keep the block below. You can un-indent the block if you like.

Hey Ben yes I thought this looked a little strange but when I close the MOZ_ASSERT with a bracket i.e. do this .. 
MOZ_ASSERT(s->IsBrowserStream()) 
{ BrowserStreamParent* sp = ....etc}

the compiler flags and error saying there is a ';' missing. I am not sure why this is? I thought that the assert statement should return a bool and then then following block should be excecuted depending if it is true or false. can you paste the code here how you think it should work? 
I would like to get the patch sumbitted myself :)

Thanks again Dan

Comment 24

[Benjamin Smedberg]

> Hey Ben yes I thought this looked a little strange but when I close the
> MOZ_ASSERT with a bracket i.e. do this .. 
> MOZ_ASSERT(s->IsBrowserStream()) 
> { BrowserStreamParent* sp = ....etc}
> 
> the compiler flags and error saying there is a ';' missing. I am not sure
> why this is? I thought that the assert statement should return a bool

Aha, that is incorrect. Assertions are statements and do not have any value. So in this case you want the assertion by itself:

MOZ_ASSERT(s->IsBrowserStream());

The remaining code doesn't need braces or to be in a block at all, it can just be plain:

BrowserStreamParent* sp = ....etc

(but you should un-indent it).

Comment 25

[dan1bh]

Comment on attachment 8883084 [details]
Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154042/diff/1-2/

Comment 26

[dan1bh]

I've made the changes Ben so hopefully this can be pushed now? Thanks, Dan

Comment 27

[Benjamin Smedberg]

Attachment: Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,

Removed API List:
* NPN_NewStream
* NPN_Write
* NPN_DestroyStream
* nsNPAPIStreamWrapper
* nsPluginStreamToFile
* PPluginStream.ipdl

Review commit: https://reviewboard.mozilla.org/r/156242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156242/

Comment 28

[Benjamin Smedberg]

Attachment: Remove a tests for functionality that we're removing either in bug 1352559 (NPN_NewStream) or bug 1352567 (seekable and/or file streams).

Review commit: https://reviewboard.mozilla.org/r/156244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156244/

Comment 29

[Benjamin Smedberg]

Comment on attachment 8885395 [details]
Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,

https://reviewboard.mozilla.org/r/156242/#review161374

This patch was by dan1bh and was originally at https://reviewboard.mozilla.org/r/154042/ - I updated it and reuploaded it here.

Comment 30

[Benjamin Smedberg]

I took this patch and did a local test build: I had to fix a bunch of build errors and some test failures.

I've now pushed a try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d099f5acdeb6953b47280bdcac98a81a990e061

Comment 31

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8885396 [details]
Remove a tests for functionality that we're removing either in bug 1352559 (NPN_NewStream) or bug 1352567 (seekable and/or file streams).

https://reviewboard.mozilla.org/r/156244/#review161416

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f24b86221837
Prequel to bug 1352559 - #includes and forward declarations necessary for various files to build properly exposed by unified build changes, r=trivial
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d7da5a9c91d
Remove tests for functionality that we're removing either in bug 1352559 (NPN_NewStream) or bug 1352567 (seekable and/or file streams). r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/999aa7b6683d
Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery, r=bsmedberg

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage: unused variable 'stillwaiting' at dom/plugins/test/testplugin/nptest.cpp:1409:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ca01a17858c522f78a6ebe1c6db4dd09819c8da
https://hg.mozilla.org/integration/mozilla-inbound/rev/f239f27f8a3b764b76cf1ac394e74e52cc953c44
https://hg.mozilla.org/integration/mozilla-inbound/rev/28f351ecb49b91cba62b4be6ed222657a4b01143

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=999aa7b6683d7f0cf481f8dd0e8a9ba3dade4a05&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=113699243&repo=mozilla-inbound
>  /home/worker/workspace/build/src/dom/plugins/test/testplugin/nptest.cpp:1409:10: error: variable 'stillwaiting' set but not used [-Werror=unused-but-set-variable]

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21899ff0548e
Prequel to bug 1352559 - #includes and forward declarations necessary for various files to build properly exposed by unified build changes, r=trivial
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca840114cd92
Remove tests for functionality that we're removing either in bug 1352559 (NPN_NewStream) or bug 1352567 (seekable and/or file streams). r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1548354e0c
Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery, r=bsmedberg

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21899ff0548e
https://hg.mozilla.org/mozilla-central/rev/ca840114cd92
https://hg.mozilla.org/mozilla-central/rev/cb1548354e0c

Comment 36

[Nicholas Nethercote [inactive]]

This is causing major crash problems, alas; see bug 1381240.

Comment 37

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/aff336ac161d
Backed out changeset cb1548354e0c (bug 1352559, part 3) for causing *many* crashes in Nightly. r=backout

Comment 38

[Benjamin Smedberg]

Attachment: bug1352559-part4.patch

We removed all the external callers of NPN_DestroyStream, but there was one internal caller which was primarily being triggered during plugin teardown. This patch gets rid of that caller (it just ignores incoming data until the stream is completely torn down), and the related IPDL message which is no longer necessary.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a066ca85c0
Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery, r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/745cf0c90bd7
followup (part 4) - remove remaining references to NPN_DestroyStream called by internal code on stream teardown, r=qdot

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87a066ca85c0
https://hg.mozilla.org/mozilla-central/rev/745cf0c90bd7