98641 - Implement passing NPSavedData arg between NPP_Destroy() and NPP_New()

Status: RESOLVED WONTFIX

Description

[av (gone)]

This bug is an off-spring of bug 85549, I filing this one as it is more specific 
in what the real problem is and more general in what the effect of it could be.

Currently we just ignore the save_data parameter when we create a new plugin 
instance always passing null. This causes e.g. Acrobat reader not to remember 
the scroll position when we leave the page with a pdf document and come back. 
Save_data is a general mean the plugin can use to restore anything it would like 
to when user returns to the page with it.

Comment 1

[av (gone)]

*** Bug 85549 has been marked as a duplicate of this bug. ***

Comment 2

[av (gone)]

Sorry, forgot formal thing. Steps for reproduction and verification purposes:

1. load the above url, this is several page long pdf document
2. scroll it all the way down, there are some links at the bottom
3. click on one of them and leave the original document
4. click the browser Back button to return to the original document

Actual: the document displayed with the first page visible
Expected: the document should be displayed as we leave it -- last page in view. 
4.x does it right.

Comment 3

[Peter Lubczynski]

Nominating nsbeta1 and mozilla1.0. This is one feature of the NPAPI still left
to be implemented but it's mentioned in the new SDK docs. Plus, Acrobat uses it.

Comment 4

[lmcquarr]

Acrobat definitely needs this.  It allows us to go back to the last
page a user was viewing in a particular PDF file.   (This works currently
in Netscape 4.X and Mac IE.  It does not work in Win IE, however.)

Comment 5

[Radha on family leave (not reading bugmail)]

In NS4.x  I think it was a big hack to get it to work that way. I implemented a
similar feature in NS4.x for java applets and it was indeed a big hack to make
the  applets restore their previous state when going back or forward to them.
I'm not sure about the feasibility of  doing the same in NS6.x. But I think it
can be achieved by plugins maintaining some sort of state information about the
child. 

Session History does have an attribute to maintain scrollbar state.
nsILayoutHistoryState (member of nsISHEntry) maintains all layout related state
information including scrollbar position, form values etc .... for regular
pages. The layoutHistoryState maintained by session history probably doesn't
kick in when a plugin is displayed. It may be worth studying the possibility of
using this member for plugins too. 

Comment 6

[rubydoo123]

nsbeta1+ per ADT meeting today

Comment 7

[Winnie Lam]

adding adt3 to status whiteboard as per discussion with beppe.  this is not a
crasher, more like a nuisance to user.

Comment 8

[av (gone)]

I am implementing the mechanism to save saveddata pointers when we leave a page
with a plugin and will post a patch soon. Meanwhile, would appreciate ideas on
how to handle multiple plugins on a page. In general case we may have different
plugins or different instances of the same plugin and all of them may want to
use saveddata. Any thoughts how we can remember associations and hand right
pointer to right instance when we return to the page?

Comment 9

[av (gone)]

Created attachment 78842 [details] [diff] [review]
patch v.1

This patch lacks Mac build script changes, I need help with this.

Please try on Windows and Unix and review.

Comment 10

[av (gone)]

This patch is rather big, so I will add couple of words about what it does.

I added a new global object |nsPluginHistory| which keeps track of saved data
pointers. |nsPluginHistory| is created and deleted in |nsPluginHostImpl|
ctor/dtor and is passed to every instance of |ns4xPluginInstance| object where
it is stored as a member. There is no particular reason not to make it under the
ownership of the plugin host, but I chose not to add more mess to our huge and
hairy plugin host class. |nsPluginHistory| keeps a list of visited URLs (pages
with plugins), each element of this list is in turn a list of saved data
pointers as one URL may have multiple plugins or plugin instances. Both kinds of
lists are objects of |nsVoidArray| type. There is a limit on the number of
stored URL entries, but there is no limit on the number of saved data pointers
for each URL. I decided always to store |sdata|, whether plugin sets it or not,
otherwise I see no easy way to determine which plugin on the page wants its data
back and which does not.
Please comment and review.

Comment 11

[serge (gone)]

the spec on this issue confuses me, it has an example:
"a video player could save the last frame number to be displayed. When the user 
returns to the page, the previous frame number is passed to the new instance of 
the plug-in, so it can initially display the same frame."
what if page has 2 instances of the same player plugin and both want to save its 
data?

Comment 12

[Peter Lubczynski]

What about using the browser's session history?

Comment 13

[av (gone)]

To peterl: nsISHistory is frozen, I did not look at it in depth but found no
obvious way to bind plugin data to it. Tracking plugin data is pretty trivial
task, so implementing it separately would not take much more time than finding a
proper way of hooking it to the session history. In addition, we have an
advantage of ownership. In case we need more complicated mechanism of
manipulating the history entries which the session history already has and find
a way to feed it with our data then I think it will be easy to hook up not even
changing |nsPluginHistory| API.

To serge: I don't quite understand what you are asking. If you mean what is
going to happen in such a case according to the old spec, then I do not know. If
your question is about the patch, then both data will be saved and restored when
needed. Here I assume that embedded objects are created and destroyed in the
same order. Which may be not the case, then we will have a bug and will need to
find a way to create associations.

Comment 14

[serge (gone)]

>according to the old spec
is there new one?
>Here I assume that embedded objects are created and destroyed in the same order
And you also assumed the embedded objects are going to be the same for the given 
url, is this correct?
>then we will have a bug and will need to find a way to create associations
why do not implement it now, to avoid future bugs?

Comment 15

[av (gone)]

>And you also assumed the embedded objects are going to be the same for the given 
>url, is this correct?

No. I don't assume that. There are comments in the patch about this. It will
discard the data in such a case.

>>then we will have a bug and will need to find a way to create associations
>why do not implement it now, to avoid future bugs?

Any ideas? Besides, the assumption could be true.

Comment 16

[Peter Lubczynski]

Created attachment 79290 [details] [diff] [review]
patch to Mac XML project to add nsPluginHistory.cpp

Comment 17

[av (gone)]

Can we raise an ADT status of the bug? I think this is kind of important, and
given there already is a working patch, we probably don't want to loose the
functionality.

Comment 18

[rubydoo123]

It has the appropriate setting, but I do not see an r=, sr= or drivers approval

Comment 19

[av (gone)]

What I mean, AFAIK today is the last day when ADT3 bugs will be allowed to go
in. This one still needs work.

Comment 20

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 21

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 22

[rubydoo123]

moving to 1.1 alpha

Comment 23

[rubydoo123]

The plug-ins triage team (av, beppe, peterl, serge and shrir) have reviewed this
issue and have made the following determination:

This is necessary and will be addressed within the next milestone, the patch
needs to be reviewed and modified as required.

Comment 24

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 25

[rubydoo123]

moving to 1.4 beta, plug-in branch work

Comment 26

[Peter Lubczynski]

jkesier gave me a strategy to use session history,
--->peterl

Comment 27

[Peter Lubczynski]

per topembed+ triage: P2

Comment 28

[William Bardwell]

A further issue is that as the current code stands it doesn't
even delete the NPSavedData fully, so this leaks memory for
any plugin that tries to use this functionality.
The following should be done until this bug is fixed for real:
--- embedding/browser/activex/src/pluginhostctrl/nsPluginHostCtrl.cpp
@@ -725,6 +725,7 @@
         if (pSavedData && pSavedData->buf)
         {
             NPN_MemFree(pSavedData->buf);
+            NPN_MemFree(pSavedData);
         }
     }

William Bardwell
[e-mail]

Comment 29

[William Bardwell]

Created attachment 129036 [details] [diff] [review]
Fix for memory leakage in current NPSavedData handling

Please apply...and then do the hard work of fixing the bug for real...

Comment 30

[William Bardwell]

It still leaks the NPSavedData object, although that code seems to have moved.

Comment 31

[Josh Aas]

I have never heard any mention of this feature in all my years of working on NPAPI specs and Mozilla's implementation . It isn't a commonly requested feature and it doesn't seem necessary. NPAPI plugins are trusted, they can store their own state in memory or on disk.

I'm open to arguments for doing this, but right now this seems like WONTFIX to me.

Comment 32

[Bastien Nocera]

(In reply to Josh Aas (Mozilla Corporation) from comment #31)
> I have never heard any mention of this feature in all my years of working on
> NPAPI specs and Mozilla's implementation . It isn't a commonly requested
> feature and it doesn't seem necessary. NPAPI plugins are trusted, they can
> store their own state in memory or on disk.

Could it possibly be because it doesn't work? If it did work, people would have use it. Nice API though:
https://developer.mozilla.org/en/NPSavedData
Shame it doesn't work as documented:
https://developer.mozilla.org/en/NPP_New