Closed Bug 98641 Opened 23 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: serhunt, Assigned: peterl-bugs)

References

()

Details

(Keywords: testcase, topembed+, Whiteboard: [PL:BRANCH],edt_a3,edt_c3)

Attachments

(3 files)

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.
*** Bug 85549 has been marked as a duplicate of this bug. ***
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.
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.
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.)
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.
nsbeta1+ per ADT meeting today
Keywords: nsbeta1nsbeta1+
adding adt3 to status whiteboard as per discussion with beppe. this is not a crasher, more like a nuisance to user.
Whiteboard: [ADT3]
Blocks: 55959
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?
Attached patch patch v.1 (deleted) — Splinter Review
This patch lacks Mac build script changes, I need help with this. Please try on Windows and Unix and review.
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.
Status: NEW → ASSIGNED
Keywords: patch
Priority: -- → P3
Target Milestone: --- → mozilla1.0
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?
What about using the browser's session history?
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.
>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?
>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.
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.
It has the appropriate setting, but I do not see an r=, sr= or drivers approval
What I mean, AFAIK today is the last day when ADT3 bugs will be allowed to go in. This one still needs work.
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1-
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1+
moving to 1.1 alpha
Target Milestone: mozilla1.0 → mozilla1.1alpha
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.
Whiteboard: [ADT3] → [PL2:NA]
Target Milestone: mozilla1.1alpha → mozilla1.0.2
Keywords: nsbeta1-nsbeta1+
Target Milestone: mozilla1.0.2 → mozilla1.2alpha
Keywords: topembed
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
Keywords: topembedtopembed+
Blocks: grouper
moving to 1.4 beta, plug-in branch work
Whiteboard: [PL2:NA] → [PL:BRANCH]
Target Milestone: mozilla1.3alpha → mozilla1.4beta
jkesier gave me a strategy to use session history, --->peterl
Assignee: av → peterl
Status: ASSIGNED → NEW
per topembed+ triage: P2
Priority: P3 → P2
QA Contact: shrir → ashishbhatt
Whiteboard: [PL:BRANCH] → [PL:BRANCH],edt_a3,edt_c3
Target Milestone: mozilla1.4beta → mozilla1.5alpha
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 wbardwel@curl.com
Please apply...and then do the hard work of fixing the bug for real...
It still leaks the NPSavedData object, although that code seems to have moved.
Depends on: 396123
Hardware: PC → All
Target Milestone: mozilla1.5alpha → ---
QA Contact: ashshbhatt → plugins
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
(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
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: