Closed Bug 1352559 Opened 7 years ago Closed 7 years ago

Remove NPN_NewStream and related machinery

Categories

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

Product:
Core Graveyard
Component:
Plug-ins
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: benjamin, Assigned: dan1bh, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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
59 bytes, text/x-review-board-request
benjamin
: review-
Details
Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352559 - Remove support for plugin-provided streams; NPN_NewStream, PPluginStream and other supporting machinery,
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Remove a tests for functionality that we're removing either in bug 1352559 (NPN_NewStream) or bug 1352567 (seekable and/or file streams).
59 bytes, text/x-review-board-request
qdot
: review+
Details
bug1352559-part4.patch
6.08 KB, patch
qdot
: review+
Details | Diff | Splinter Review 
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.
Blocks: 1352567
It looks like we can't remove nsNPAPIStreamWrapper in this bug; it will have to wait for bug 1352567
No longer blocks: 1352567
Blocks: 1352567
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.
Flags: needinfo?(benjamin)
Hi Benjamin can I be assigned to this? Or is there another mentored bug that I could work on?
(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.
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
Flags: needinfo?(benjamin)
Oh I forgot can you point me too the tests that I should run for this? Thanks again
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.
Flags: needinfo?(benjamin)
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?
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?
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.
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.
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
Flags: needinfo?(benjamin)
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?
Flags: needinfo?(benjamin)
Thanks dan! I've marked this bug as assigned to you and I'll have a look over this shortly.
Assignee: nobody → 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

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
Attachment #8872360 - Flags: review?(benjamin) → review-
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?
I've implemented these changes Ben but I've some questions for you - I have emailed them over today! thanks Dan
Flags: needinfo?(benjamin)
Answered email, I hope that's enough!
Flags: needinfo?(benjamin)
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
Attachment #8883084 - Flags: review+
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
> 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).
I've made the changes Ben so hopefully this can be pushed now? Thanks, Dan
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.
Attachment #8885395 - Flags: review?(benjamin) → review+
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 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
Attachment #8885396 - Flags: review?(kyle) → review+
Pushed by bsmedberg@mozilla.com:
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
Pushed by bsmedberg@mozilla.com:
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
Flags: needinfo?(benjamin)
This is causing major crash problems, alas; see bug 1381240.
Blocks: 1381240
Backout by nnethercote@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/aff336ac161d
Backed out changeset cb1548354e0c (bug 1352559, part 3) for causing *many* crashes in Nightly. r=backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8887133 - Flags: review?(kyle)
Attachment #8887133 - Flags: review?(kyle) → review+
Pushed by bsmedberg@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/87a066ca85c0
https://hg.mozilla.org/mozilla-central/rev/745cf0c90bd7
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Depends on: 1523390
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: