345339 - Revisit nsPresState

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

nsPresState objects remember frame-specific and element-specific data in the session history entry for a page, e.g., the offset to which a scrollable frame is scrolled. They're implemented as a hashtable from strings to XPCOM objects. This is grossly inefficient because these objects are used in very limited ways (use lxr to search for "nsPresState"), except for XUL nsBoxObject which uses them for storing arbitrary properties for the nsBoxObject JS-accessible property API.

I think we should change boxobjects to use a hashtable directly and change the other nsPresState users to use something simpler. The trickiest part is that there can be state from content and also state from frames (nsGfxScrollFrame) put into the same nsPresState object. One option would be to make nsPresState just a pair of nsAutoPtr<nsScrollState> and nsCOMPtr<nsISupports>, where nsScrollState is the state from nsGfxScrollFrame, or null if there isn't any, and the nsISupports is whatever the element needs to store, if any (e.g. nsISupportsString).

Comment 1

[Karthik Sarma]

Attachment: Patch for several files in layout/, adds a nsScrollState class, modifies nsBoxObject to not use nsPresState

The patch adds a nsScrollState class to keep track of what nsGfxScrollFrame needs to save (nsGfxScrollFrame still uses nsPresState which holds a ptr to a Scroll State). nsBoxObject is modified to not use nsPresState at all and instead use a member hashtable of its own. The hashtable in nsPresState is still used by clients other than nsScrollState and nsBoxObject.

Incidentally, this is my first patch that does anything nontrivial, so it is probably very bad :/

Comment 2

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

Comment on attachment 238727 [details] [diff] [review]
Patch for several files in layout/, adds a nsScrollState class, modifies nsBoxObject to not use nsPresState

When you ask for review, always ask for review from someone specific. Otherwise no-one will ever hear about your request.

Comment 3

[Karthik Sarma]

(In reply to comment #2)
> (From update of attachment 238727 [details] [diff] [review] [edit])
> When you ask for review, always ask for review from someone specific. Otherwise
> no-one will ever hear about your request.
> 

Ups... that would explain a lot :) Thanks!

Comment 4

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

This is good stuff.

Don't copy the evil "nsresult NS_NewScrollState(nsScrollState** aScrollState)" pattern. Just do the simple thing: make nsScrollState be a struct defined in nsPresState.h. It doesn't even need any methods except perhaps a constructor that sets everything to zero (inline!). Then there's no need for nsScrollState.cpp.

+  nsCOMPtr<nsISupports> mISupports;

You're not using this yet, so don't add it yet.

Instead of 8 methods in nsPresState, I would just have two:
  void SetScrollState(PRInt32 aX, PRInt32 aY, PRInt32 aWidth, PRInt32 aHeight);
  const nsScrollState* GetScrollState();

   nsAutoString result;
-  nsresult rv = mPresState->GetStateProperty(propertyName, result);
+  nsresult rv = NS_STATE_PROPERTY_NOT_THERE;
+  result.Truncate();

You don't need to truncate, it's already empty.

+  if(supportsStr) {

Space after "if".

+    nsCAutoString data;
+    supportsStr->GetData(data);

Don't shadow a variable you've already introduced with the same name.

Why are you storing nsISupportsCStrings? To save memory using UTF8? I would just use nsISupportsString, it's simpler ... until we have some evidence that any significant amount of memory is used for these strings.

+#include "nsStringFwd.h"

I think you want nsHashKeys.h.

+#include "nsLayoutErrors.h"

You don't need this.

+  nsInterfaceHashtable<nsStringHashKey,nsISupports> mPropertyTable; // [OWNER]

These hashtables are fairly heavyweight. The minimum size you can actually allocate is 16. I suggest we heap-allocate these on demand, like mPresState used to be, so make this an nsAutoPtr<nsInterfaceHashtable<nsStringHashKey,nsISupports> >.

Comment 5

[Karthik Sarma]

Attachment: Rewrite of above patch

Thanks for the comments, Robert. I changed things around to use structs and to allocate the hashtables on demand (as well as the other changes). I'm not sure that I allocated the hashtables properly -- I looked through lxr for examples but I did not see a clear pattern, so please let me know if there is a better way to do it.

Comment 6

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

+    nsScrollState** newScrollState = getter_Transfers(mScrollState);
+    *newScrollState = new nsScrollState();

Just write "mScrollState = new nsScrollState();"

+  if (!mScrollState) return nsnull;
+  return mScrollState;

Just "return mScrollState;"

+  if (!aState) return;

Not needed.

+  PRInt32 xoffset;
+  PRInt32 yoffset;
+  PRInt32 width;
+  PRInt32 height;
+  
+  const nsScrollState* oldState = aState->GetScrollState();
+  if (!oldState) return;
+  xoffset = oldState->mXOffset;
+  yoffset = oldState->mYOffset;
+  width = oldState->mWidth;
+  height = oldState->mHeight;

Just declare these variables where they're assigned, to save some lines. Actually, don't bother with them at all, just use oldState->... directly.

+struct nsScrollState;

Why do you need that?

+  if (mPropertyTable)
+    mPropertyTable->Clear();

Just do "mPropertyTable = nsnull;" to delete the table completely if it's present.

+    nsInterfaceHashtable<nsStringHashKey,nsISupports>** newTable = 
+      getter_Transfers (mPropertyTable);
+    *newTable = new nsInterfaceHashtable<nsStringHashKey,nsISupports>;

Again, just do mPropertyTable = new ...

+    if (NS_FAILED(mPropertyTable->Init(8))) return NS_ERROR_FAILURE;

On failure, set mPropertyTable to nsnull before returning, otherwise you might try to use the uninitialized mPropertyTable next time.

-  if (!mPresState) {
-    *aResult = nsnull;
-    return NS_OK;
-  }
+  if (!mPropertyTable) return NS_ERROR_FAILURE;  

You should set *aResult to null and then return NS_OK, just like we currently do.

+  nsDependentString result;
+  nsresult rv = NS_ERROR_FAILURE;
+  nsISupports *data = mPropertyTable->GetWeak(propertyName);
+
+  nsCOMPtr<nsISupportsString> supportsStr = do_QueryInterface(data);
+  if (supportsStr) {
+    nsAutoString inString;
+    supportsStr->GetData(inString);
+    rv = NS_OK;
+  }

I'm not really sure what you're trying to do here, but it doesn't work. inString just gets thrown away. I think you want to declare 'result' as an nsAutoString and call GetData(result).

I suggest that you rewrite GetProperty and SetProperty to just call GetPropertyAsSupports/SetPropertyAsSupports, wrapping the string into/out of nsISupportsString. That would save code here. Make sense?

nsBoxObject::RemoveProperty(const PRUnichar* aPropertyName)
{
   NS_ENSURE_ARG(aPropertyName && *aPropertyName);
-  if (!mPresState)
-    return NS_OK;
+
+  if (!mPropertyTable) return NS_ERROR_FAILURE;

This should return NS_OK like the code currently does. Avoid changing behaviour for no reason.

Comment 7

[Karthik Sarma]

Attachment: Bug fixes and rewrite

I rewrote it so it just reuses the AsSupports methods. I believe everything returns as expected now as well.

Comment 8

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

+const
+nsScrollState*

One line

+  nsScrollState()
+  {
+    mXOffset = 0;
+    mYOffset = 0;
+    mWidth = 0;
+    mHeight = 0;
+  }

Don't do this, it's not needed.

Actually, what would be even better would be to eliminate ScrollState entirely. Just use nsRect. Make SetScrollState take a const nsRect&.

+  const nsScrollState* GetScrollState();

Should be NS_HIDDEN_()

+  if (!mPropertyTable)
+  {

One line

+  if (!mPropertyTable){
+  

Space before {, no empty line

+    if (NS_FAILED(mPropertyTable->Init(8)))
+    {

One line

+  if (NS_FAILED(GetPropertyAsSupports(aPropertyName,getter_AddRefs(data))))
+  {

One line

+  nsCOMPtr<nsISupportsString> supportsStr = do_QueryInterface((nsISupports*)data);

You should not need the cast here.

+  nsresult rv = NS_ERROR_FAILURE;
+  nsCOMPtr<nsISupportsString> supportsStr = do_QueryInterface((nsISupports*)data);
+  nsAutoString result;  
+  if (supportsStr) {
+    supportsStr->GetData(result);
+    rv = NS_OK;
+  }

Probably better to just do
if (!supportsStr)
  return NS_ERROR_FAILURE;
and delay declaring rv

 
+

Don't need this new blank line between functions

+  nsCOMPtr<nsISupportsString> supportsStr(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));

If you #include "nsSupportsPrimitives.h" you can just write
  nsCOMPtr<nsISupportsString> supportsStr = new nsSupportsString();

Comment 9

[Karthik Sarma]

Attachment: uses nsRect and style fixes

Style fixes and uses nsRect. I couldn't get nsCOMPtr<nsISupportsString> supportsStr = new nsSupportsString() to work even with the new include, though.

Comment 10

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

Give mRect a proper name, like mScrollState, and make it a pointer to a rect so we don't allocate it if we don't need it.

+    mPropertyTable = new 
+nsInterfaceHashtable<nsStringHashKey,nsISupports>;  

Don't indent like this. The second line should be indented more than the line before it.

> I couldn't get nsCOMPtr<nsISupportsString>
> supportsStr = new nsSupportsString() to work even with the new include, though.

What's the error?

Comment 11

[Karthik Sarma]

Attachment: more style fixes, allocates mScrollState rect on demand

The build errors I'm getting are:
layout/xul/base/src/nsBoxObject.cpp: In member function 'virtual nsresult nsBoxObject::SetProperty(const PRUnichar*, const PRUnichar*)':
layout/xul/base/src/nsBoxObject.cpp:421: error: expected type-specifier before 'nsSupportsString'
layout/xul/base/src/nsBoxObject.cpp:421: error: conversion from 'int*' to non-scalar type 'nsCOMPtr<nsISupportsString>' requested
layout/xul/base/src/nsBoxObject.cpp:421: error: expected ',' or ';' before 'nsSupportsString'

lxr does not have a match for the identifier nsSupportsString -- there is a nsSupportsStringImpl but it can't be instantiated in any way I can find. I assume that is the problem but I'm not sure what string type to use here.

Comment 12

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

+  if (!mScrollState) mScrollState = new nsRect();
+  *mScrollState = aRect;

Check for out-of-memory.

+nsresult
+nsPresState::GetScrollState(nsRect& aRect)
+{
+  if (!mScrollState) return NS_ERROR_FAILURE;

You could return an nsRect directly, and when mScrollState is null, return (0,0,0,0).

+  mRestoreRect.SetRect(-1, -1, -1, -1);
+  // don't do it now, store it later and do it in layout.
+  mRestoreRect.SetRect(oldState.x, oldState.y, 
+oldState.width, oldState.height);

Why are you calling SetRect twice? With the above fix, you can just write
  mRestoreRect = aState->GetScrollState();

Ah, you need to do "new nsSupportsStringImpl", that's what the class is called (see http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsSupportsPrimitives.h )

Comment 13

[Karthik Sarma]

Attachment: couple of small fixes and tries to use nsSupportsStringImpl (does not compile)

Unfortunately I can't get that one to work either. This time I'm getting a vtable error but I don't see any missing includes or anything...

../xul/base/src/libgkxulbase_s.a(nsBoxObject.o): In function `nsSupportsStringImpl':
../../../../dist/include/xpcom/nsSupportsPrimitives.h:88: undefined reference to `vtable for nsSupportsStringImpl'
collect2: ld returned 1 exit status

Comment 14

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

Alright, go back to the CONTRACTID version and I'll call it done.

Comment 15

[Karthik Sarma]

Attachment: uses CONTRACTID to allocate string

Comment 16

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

Comment on attachment 243112 [details] [diff] [review]
uses CONTRACTID to allocate string

yay!

Comment 17

[Karthik Sarma]

(In reply to comment #16)
> (From update of attachment 243112 [details] [diff] [review] [edit])
> yay!
> 

Yup...

Now what? heh

Comment 18

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

Get on irc.mozilla.org #developers to get someone to check in for you.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I've landed the patch on the trunk. Should this bug now be marked FIXED?

mozilla/layout/xul/base/public/nsPIBoxObject.h 1.9
mozilla/layout/base/nsPresState.cpp 3.27
mozilla/layout/base/nsPresState.h 3.4
mozilla/layout/xul/base/src/nsBoxObject.cpp 1.65
mozilla/layout/xul/base/src/nsBoxObject.h 1.19
mozilla/layout/generic/nsGfxScrollFrame.cpp 3.272

Comment 20

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

Let's leave this open.

The next step is to examine the users of the Get/SetStateProperty* methods and replace those methods with something else, so we can remove the mPropertyTable. I think we should do it like this:

1) Move the SaveState/RestoreState code out of nsIsIndexFrame into nsHTMLSharedElement (for ISINDEX elements).

2) Then replace the mPropertyTable with a single "nsCOMPtr<nsISupports> mContentState". The various element classes that implement SaveState/RestoreState would set this to whatever they need. In most cases they only need to set one value so this works fine (using nsISupportsString or whatever other wrapper class is needed, if any). nsHTMLInputElement is a bit special, it works with four state fields ("checked", "v" (for value), "f" (for filename", and "disabled"), and we should keep those separate. So for nsHTMLInputElement we'll need to define a new struct implementing nsISupports that contains those four fields.

Comment 21

[Paul Tomlin]

Not sure if this is intended or expected, but I think this checkin triggered bug 359773. The depends/blocks on that bug may be the wrong way round too.

Comment 22

[[e-mail]]

Comment on attachment 243112 [details] [diff] [review]
uses CONTRACTID to allocate string

>+  nsCOMPtr<nsISupportsString> supportsStr = do_QueryInterface(data);
>+  if (!supportsStr) 
>+    return NS_ERROR_FAILURE;
I'm not sure this is ideal; iirc GetProperty never used to throw.

>+  nsAutoString result;  
>+  supportsStr->GetData(result);
>+
>   *aResult = result.IsVoid() ? nsnull : ToNewUnicode(result);
>   return NS_OK;
You can actually write
return supportsStr->ToString(aResult);

Comment 23

[Karthik Sarma]

Attachment: Removes the hashtable from nsPresState

This patch removes the need for the hashtable from nsPresState, instead just storing one reference to an nsISupports and a PRBool for saving disabled status

It doesn't move the save/restore code out of nsIsIndexFrame... wasn't quite sure what you were asking for there, but it should be an easy change

I added a new class implementing nsISupports for the HTMLInputElements' state. I'm pretty sure I have the implementation right. Unfortunately, though, this patch doesn't work. It'll compile but it freezes on startup. I've traced the problem down to the call by nsHTMLInputElement::SaveState() method to nsPresState::SetStatePropertyAsSupports() and the problem is somewhere in xpcom, I think. Maybe I'm not allocating memory correctly for the nsHTMLInputElementState in SaveState() ? Most nsISupports objects use do_CreateInstance, but we don't have a CONTRACTID for this new class...

So, I'm not entirely sure where to go from here. Maybe someone else can shed some light?

Comment 24

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

+    nsCOMPtr<nsISupports> mValue;
+    nsCOMPtr<nsISupports> mFilename;

These can just be nsStrings directly, they don't need to be refcounted objects.

Why does nsHTMLInputState need a "disabled" field? We already have one in nsPresState.

+nsHTMLInputElementState::SetDisabled(const PRBool& aDisabled)

Don't use const PRBool&; just "PRBool" is simpler and more efficient. Also, just return void, since these setters can't fail.

+    NS_HIDDEN_(nsresult) GetDisabled(PRBool* aDisabled);

Document that you need to return an nsresult because this can fail with NS_STATE_PROPERTY_NOT_THERE.

+  if (aDisabled != PR_TRUE && aDisabled != PR_FALSE) return NS_ERROR_FAILURE;

Don't compare to booleans. And don't write this code; you can assume that any PRBool is PR_TRUE or PR_FALSE.

+  if (mChecked == -1) return NS_STATE_PROPERTY_NOT_THERE;

Can these properties ever not be there? Presumably we only restore state that we previously saved, and if we saved it, the value should be there... but I guess maybe the form control type could change in between. OK. But then you need to check the return value every time. 

Comment 25

[Karthik Sarma]

Attachment: Update: removes the Hashtable from nsPresState

This implements the various changes you asked for. It also adds an interface nsIHTMLInputElementState that extends nsISupports through XPCOM -- I'm not sure if this is overkill in this case, so let me know what you think.

Comment 26

[Karthik Sarma]

Actually, on further consideration, it occurs to me that perhaps it's pointless to expose any of the nsPresState functionality through XPCOM -- I don't see any other relevant usages, so perhaps just making nsHTMLInputElementState a standalone struct and not not using a COMPtr to store the internal data in nsPresState would be more efficient?

Comment 27

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

Right.

Comment 28

[Karthik Sarma]

Attachment: Removes the hashtable from nsPresState

I've added a state object for each of the HTML elements that will be stored in nsPresState -- this should cut down on the footprint. I've also removed the XPCOM bindings for nsSelectState -- I can't find any usages, and it doesn't seem like the XPCOM interface is necessary at all. I can add it back in if anyone thinks otherwise.

Comment 29

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

+    nsIsIndexFrameState() {};
+    virtual ~nsIsIndexFrameState() {};

Remove these, they'll be generated automatically.

+    nsString GetState() {

Make this return const nsString&.

+    nsAutoPtr<nsIsIndexFrameState> sState(new nsIsIndexFrameState());

This doesn't need to be nsAutoPtr. Also, sState should be called something else, say 'state'.

+    nsAutoPtr<nsPresContentState> pState(sState.forget());
+    (*aState)->SetStateProperty(pState);

I'm not sure why you're doing this. Just call SetStateProperty(state)

I think you do have to deal with the possibility that the RestoreState might be for a different frame than the SaveState, so you have to check that the state object is of the right type. This is why I suggested making it an nsCOMPtr<nsISupports> --- you can then QueryInterface it to the desired type and check if that QI succeeds. Instead of creating nsIsIndexFrameState and other classes containing a single string, just use nsISupportsString instead.

Comment 30

[Karthik Sarma]

Attachment: removes the hashtable, replaces with nsISupports classes

This patch uses QI to check that the saved state type is consistent with what is expected -- except in nsHTMLSelectElement, in which the preexisting code just used a cast instead of QI. I can change that if you'd like, but I left it the way it was in case there was a good reason (though I couldn't think of one).

Comment 31

[Karthik Sarma]

Whoops, caught a small bug -- in nsHTMLSelectElement the member variable mRestoreState should be a nsRefPtr, not a nsCOMPtr. I'll change that after review.

Comment 32

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

+  nsCOMPtr<nsHTMLInputElementState> inputState
+    (do_QueryInterface(aState->GetStateProperty()));

I'm not sure why this builds. nsHTMLInputElementState doesn't have an IID so we shouldn't be able to QI to it. Add an IID to nsHTMLInputElementState. Same for nsSelectState.

No-one uses UnsetDisabled, why is it here? Same with RemoveStateProperty.

Comment 33

[Karthik Sarma]

(In reply to comment #32)
I was wondering about that too -- I assumed there was something in the macro that added an IID automatically, since it does build and run without problems. I'll add those in.

RemoveStateProperty was in the class before but there are no usages in LXR, so I'll remove that, too.

> +  nsCOMPtr<nsHTMLInputElementState> inputState
> +    (do_QueryInterface(aState->GetStateProperty()));
> 
> I'm not sure why this builds. nsHTMLInputElementState doesn't have an IID so we
> shouldn't be able to QI to it. Add an IID to nsHTMLInputElementState. Same for
> nsSelectState.
> 
> No-one uses UnsetDisabled, why is it here? Same with RemoveStateProperty.
> 

Comment 34

[Karthik Sarma]

Attachment: Allows QI to various state classes

This should actually allow QI to the various classes -- I'm pretty sure I have it right, but since it built and ran before, too, it's possible I don't...

Comment 35

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

@@ -1,8 +1,9 @@
+
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Undo this.

+void
+nsHTMLInputElementState::SetChecked(PRBool aChecked)
+{
+  mChecked = aChecked;
+}
+
+void
+nsHTMLInputElementState::SetValue(const nsAString &aValue)
+{
+  mValue = aValue;
+}
+
+void
+nsHTMLInputElementState::SetFilename(const nsAString &aFilename)
+{
+  mFilename = aFilename;
+}
+
+PRBool
+nsHTMLInputElementState::GetChecked()
+{
+  return mChecked;
+}
+
+const nsString&
+nsHTMLInputElementState::GetValue()
+{
+  return mValue;
+}
+
+const nsString&
+nsHTMLInputElementState::GetFilename()
+{
+  return mFilename;
+}

Make these inline in the class definition.

+        nsString value = inputState->GetValue();
+
+        SetValueInternal(value, nsnull);

Don't use the temporary 'value' here, just pass inputState->GetValue() directly.

+        nsString filename =
+          inputState->GetFilename();
+
+        SetFileName(filename);

Ditto.

+  PRBool disabled = aState->GetDisabled();
+  if (disabled != -1) {
+    SetDisabled(disabled);
   }

Why are we using a PRBool as a tri-state value? That's bad --- a PRBool should always be either true or false. Use two PRPackedBools, or define a tri-state enum.

Make nsPresState::GetDisabled/SetDisabled/GetStateProperty/SetStateProperty inline in the class definition.

Comment 36

[Karthik Sarma]

Attachment: Inlined functions, some fixes

Comment 37

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

+      nsString value;

You changed this from nsAutoString ... it should probably stay nsAutoString.

Other than that, looks good!

Comment 38

[Karthik Sarma]

Attachment: Uses nsAutoString

Whoops, that was a holdover from back when I wasn't using ISupports or something...

Fixed, now

Comment 39

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

Comment on attachment 281259 [details] [diff] [review]
Uses nsAutoString

okay, I like this now.

Requesting 1.9 approval --- apart from the code cleanup and shrinkage, this should reduce space usage for forms in session history.

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't initialize a PRBool to -1, they should always be PR_TRUE or PR_FALSE. If you need a tristate, use PRInt8 or some such. This is for nsHTMLInputElementState::mChecked

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 281259 [details] [diff] [review]
Uses nsAutoString

a=me with that fixed.

Comment 42

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

checked in. Thanks!!!

Comment 43

[Boris Zbarsky [:bzbarsky]]

This made us eagerly allocate both a state struct and a nsPresState for nsHTMLInputElement, even if we have no state to save.  In addition to the memory usage, this means a lot of extra state key generation.  In situations where most of the inputs on the page have nothing to save (which is most cases, really), that seems like a lot of extra overhead...

Comment 44

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

I backed this out anyway because it broke tests.

Comment 45

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

*** 2040 INFO Running
/tests/content/base/test/test_bug398243.html...
*** 2041 ERROR FAIL | checkbox
#1 state preserved | got
false, expected true |
/tests/content/base/test/test_bug398243.html
*** 2042 ERROR FAIL | checkbox
#2 state preserved | got true,
expected false |
/tests/content/base/test/test_bug398243.html
*** 2043 ERROR FAIL | text
preserved in <input> | got
"Write something here",
expected "New value for text
input" |
/tests/content/base/test/test_bug398243.html

Comment 46

[Karthik Sarma]

Ack, I didn't actually test the most recent iteration. I must have broken something somehow.

I'll take a look, my apologies.

Comment 47

[Graeme McCutcheon [:graememcc]]

Attachment: Patch - my attempt

This hasn't been touched in a year. Assuming it's safe to look at...

This patch corrects a few bugs in the previous version:

 - takes account of Boris' comments regarding eager allocations
 
 - for controls that have a disabled state and content data, in previous patches, if the QI failed, the restore state method would quit there and then. Per current behaviour, it should still try to restore the disabled state if possible

  - restoration of "checked" status for nsHTMLInputElement checkboxes/radioboxes was always occuring, even if it was not saved in the first place

>I'm not sure why this builds. nsHTMLInputElementState doesn't have an IID so we
>shouldn't be able to QI to it. Add an IID to nsHTMLInputElementState. Same for
>nsSelectState.

Hmm. Adding in the IID declarations etc from previous patches, I found the do_QI calls in the various restore state methods failing with NS_ERROR_NO_INTERFACE. But the approach that shouldn't work seems to! (There does seem to be some precedent in the codebase eg in http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpNTLMAuth.cpp#200 there is no IID defined for nsNTLMSessionState)

Comment 48

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

+    PRBool mChecked;
+    PRBool mCheckedSet;

PRPackedBool

+    void SetValue(const nsAString &aValue) {
+     mValue = aValue;
+    }

Fix indent

+          if (!inputState) {
+            inputState = new nsHTMLInputElementState();

How can inputState be non-null here?

+  nsCOMPtr<nsHTMLInputElementState> inputState
+    (do_QueryInterface(aState->GetStateProperty()));

I guess we're finding the IID information in a base class. That seems dangerous, we may be just using the nsISupports IID and doing unsafe casts. I'd really like to fix this properly. Adding the IID declarations should work. You should be able to step through in the debugger to find out why they don't work.

While we're here, can you get rid of NS_NewPresState and have the callers just do new nsPresState directly?

Comment 49

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v2

> How can inputState be non-null here?
D'oh, fair point! Other comments addressed also.

OK. So, the problem was that with the IIDs added, in all the previous patches the macro being used for the XPCOM goo was NS_IMPL_ISUPPORTS0, which only implements QI for the IID for nsISupports. Thus, the calls to nsCOMPtr<nsHTMLInputElementState> blah (do_QueryInterface(...) and the equivalent for nsSelectState would fail. This is almost certainly what caused the input restoration test failures when the earlier version was checked in in 2007.

Likewise, when I removed the state IIDs it worked because the above became a QI of nsISupports to nsISupports, which didn't get us any further forward as far as checking we got the right type of object back.

Hence, NS_IMPL_ISUPPORTS1(nsSelectState, nsSelectState) and the equivalent for nsHTMLInputElement, as this builds the correct QI using both IIDs. Although it does seem like syntax abuse...

This patch adds tests for the various bits I don't see already tested: disabled state is tested in the tests for 277724, and textareas/text inputs/checkboxes in 398243's mochitest.

Wasn't sure if how I removed NS_NewPresState in nsGfxScrollFrame is correct, as I didn't understand the nsAutoPtr/getter_transfers in the first place...

Comment 50

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

Duh, I should have seen the issue with ISUPPORTS0

Comment 51

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

Comment on attachment 352439 [details] [diff] [review]
Patch v2

This is awesome

Comment 52

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/89840ed77dc8

Marking in-testsuite+, though the tests don't really test _this_ bug per se.

Comment 53

[Graeme McCutcheon [:graememcc]]

Attachment: Patch that applies cleanly on branch

Now that 215405 is on branch, this could go in too. (The new patch includes the fix for bug 483217).

Comment 54

[Boris Zbarsky [:bzbarsky]]

Why would we want this on branch?

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 368260 [details] [diff] [review]
Patch that applies cleanly on branch

Denying approval for now, since this looks like a rather large cleanup patch, and no reason was given for why to take it.  Feel free to re-request with a reason as to why it should go on branch, though.

Comment 56

[Graeme McCutcheon [:graememcc]]

> Denying approval for now, since this looks like a rather large cleanup patch,
> and no reason was given for why to take it.  Feel free to re-request with a
> reason as to why it should go on branch, though.

David, Boris, apologies for reply before now. I was just thinking of the (admittedly fairly minor, don't have a toolchain to hand to properly quantify) memory win, as outlined in the 1.9 approval request for Karthik's earlier version of the patch in comment 39.

Comment 57

[Boris Zbarsky [:bzbarsky]]

I don't think it's worth it to take this on branch for that at this stage...

Comment 58

[Henri Sivonen (:hsivonen)]

Any ideas what could make the type=file and type=hidden inputs fail the test (restoring to "") on content/base/test/test_bug345339.html while the four other checks succeed?

I'm investigating this over at bug 497861. The only difference I see in the stacks leading to SaveState is that in one case with old parser enabled, the cycle collector causes a call to SaveState by unlinking the nsHTMLDocument while the corresponding stack in the HTML5 parser case starts with the CC unlinking the body element.