264964 - shistory loads do not restore form data if only hash changed

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

scenario:
o hit reply in some mozilla bug (you are now at show_bug.cgi?id=...#add_comment)
o fill out bugzilla textarea
o submit (you are now at process_bug.cgi)
o get collision
o click "throw away changes". you are now at show_bug.cgi?id=...
o remember you wanted to keep them and choose the previous bugzilla page in the 
  back dropdown menu. URI is _almost_ the same, but hash is different.

No form data is restored.

Note: To get it back, you can go back further to some other uri and forward
again. this is somewhat non-obvious, thus marking dataloss.

Code at
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5257 is
responsible for this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162542 [details] [diff] [review]
Proposed patch

The issue is that the code scrolls in this case, since we're doing a history
load and the postdata is the same (null).

The solution is to separate the concepts of "history entries for the same page
modulo anchor traversal" and "history entries with the same postdata value". 
The former should imply the latter, but not conversely.

Comment 3

[Darin Fisher]

Comment on attachment 162542 [details] [diff] [review]
Proposed patch

>Index: xpfe/components/shistory/public/nsISHEntry.idl

>+attribute unsigned long pageIdentifier;

please update the interface uuid.  might also want to update the uuid
of any interface that references this interface directly.


>Index: docshell/base/nsDocShell.cpp

>+            PRUint32 pageIdent = PRUint32(-1);

nit: PR_UINT32_MAX maybe?


>+                if (pageIdent != PRUint32(-1))

same here.


sr=darin with interface change.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> might also want to update the uuid of any interface that references this
> interface directly.

What do you mean by "directly"?  Like "have an nsISHEntry property"?  Or "take
an nsISHEntry argument"?

> nit: PR_UINT32_MAX maybe?

Agreed.

Comment 5

[Darin Fisher]

> What do you mean by "directly"?  Like "have an nsISHEntry property"?  Or "take
> an nsISHEntry argument"?

perhaps both.  case in point is bug 234169, but i'm not sure what our policy
should really be in regards to such "interface" changes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So this is to keep people from calling a getter on an interface and getting back
an object with an unexpected vtable layout, basically?  And to keep them from
passing us such an object?

But there is nothing that says that they got the interface they're calling the
getter on from QI either.  So we'd need to propagate IID changes all the way to
the "root" interfaces (the ones that they can only get via
createinstance/getservice) or something.  That sounds pretty painful, and not
really worth it... Easier to just generate new uuids for every single unfrozen
interface with each Mozilla release via xpidl magic, imo.

Comment 7

[Darin Fisher]

I don't think we want to needlessly rev all unfrozen interface UUIDs with every
release.  We want to give people a mechanism by which they can 'safely' dip into
the unfrozen interface bucket.  If that mechanism means that they must call
QueryInterface on the result of any getter to verify the interface, then so be
it.  Otherwise, we need to update the UUID of any interface where such getters
(and setters) exist.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So we need to rev the uuid of anything that gets or sets or takes nsISHEntry. 
And the uuid of anything that gets or sets the things that get or set or take
nsISHEntry (eg nsIDocShell).  And so forth, propagating through the interface
dependency graph.

It seems to me that the set of interfaces needing revving is then very
time-consuming to determine (I'm estimating easily 10 times the time it took me
to write this patch to start with).  And it'll still end up being a fairly large
number of interfaces, I suspect....

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 162542 [details] [diff] [review]
Proposed patch

nsISHEntry.idl:
this file could use some indentation...

+	     PRUint32 pageIdent = PRUint32(-1);

PR_UINT32_MAX?

r=biesi

Comment 10

[Boris Zbarsky [:bzbarsky]]

Darin, if you really feel that I should update the iid on all interfaces that
reference nsISHEntry and all interfaces that reference them and so forth, I'll
do that this weekend, I guess....

Comment 11

[Darin Fisher]

> Darin, if you really feel that I should update the iid on all interfaces that
> reference nsISHEntry and all interfaces that reference them and so forth, I'll
> do that this weekend, I guess....

leave it for now... we need to figure out what our policy should be.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed, with the iid rev for nsISHEntry (but not anything else).  I agree we need
a general solution for this, though....