Steps to reproduce: 1. Load the attached testcase 2. Click "Go Back" 3. Click the Forward button Result: The current page continues to be shown, even though the location bar updates. Expected result: The testcase should be shown again.

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

Note, the problem only seems to occur if I save the testcase locally.

The testcase does not seem to work on secure pages like bugzilla. :)

After step 2 in your steps to reproduce, I see: Error: uncaught exception: Permission denied to set property Window.navigator in the javascript console, fwiw.

Yeah, I see a failure to copy one of the properties from the page we're going back to as well. In my case it's just a function defined in the page. It seems to be failing inside of XPC_WN_Helper_AddProperty, called from js_DefineNativeProperty. I haven't quite worked out what's failing, because of all the macro fu in that function.

It looks like it's failing the security check... I have a feeling what's happening is that it sees the JS from the testcase page on the stack and thinks that's what is trying to set the property on the other site's window.

jst can probably add value here; cc'ing bz too, since he's been into xpconnect for the XPCNativeWrapper automation work in 1.8. /be

bryner: so if nsGlobalWindow::RestoreWindowState is called with the top cx on the xpconnect thread context stack being something other than the cx used in that method (which is the nsGlobalWindow's mContext's native context), then yeah, you will hit security hassles. So temporarily push that cx on the threadcontextstack. jst prolly has a C++ auto-storage-class helper lying around. /be

Hm, I tried using the JSContextStack to push the window's JSContext before copying the properties back from the property-store object. It doesn't seem to help though. + cxStack->Push(cx); nsresult rv = CopyJSPropertyArray(cx, aSource, aDest, props); + JSContext *dummy; + cxStack->Pop(&dummy); I still get an error, "Permission denied to set property Window.sf" (sf is a function defined on http://google.com/).

Pushing a null cx may work better, actually, if we're sure that no JS will run while we have it pushed... See http://lxr.mozilla.org/seamonkey/source/layout/forms/nsTextControlFrame.cpp#3041 for an example. That said, the AddProperty classinfo hook for window will flat-out throw for some properties no matter what (eg for "location") unless sDoSecurityCheckInAddProperty is false in classinfo. So maybe what we really want here is to have the window call something on nsWindowSH to set that to false, then copy stuff, then set it back to true. Again, if we're sure nothing bad can happen in there...

Pushing cx is better than pushing null; sounds like you need an nsWindowSH friend function or two to call too, to clear and set that static. jst can weigh in, but it should be easy to do -- dom/src/base files can be tight this way without a bad modularity hazard resulting. /be

If we do the classinfo thing (which I think we should), we should have no need to push a context.

When split windows land, we can do away with copying altogether. But, in the mean time, since setters do security checks, and since that static does not control all setters' (or addProperty hooks') security checks, it seems to me better to push and pop cx. But I admit, I haven't yet identified a setter that might check and declare "access denied". /be

Well, that static (sDoSecurityCheckInAddProperty) causes us to call into ScriptSecurityManager, but I don't really understand why that security check still fails after I've pushed the window's context.

per discussion with jst, we realized that making this work this way would open a security hole, since the window would contain all of the properties of the site you're going back to after history.back() returns. I think the way to solve this is to move the window state restoration (and probably some other things) into the FinishRestore event processing instead of doing them synchronously.

Do the majority of RestorePresentation's work from a PLEvent. Seems to fix this bug, but I need to do much more testing on it.

Actually, bryner and I spoke about this patch some more and we decided that it makes sense to move two other things into RestoreFromHistory (i.e., from a PLEvent): > mEODForCurrentDocument = PR_FALSE; and the call to FirePageHideNotification.

Fixed the things Darin mentioned, plus: - Do a second CanSavePresentation check in RestoreFromHistory, just to be sure, and to more closely match the non-fastback code path. Don't save this state on the plevent anymore. - Move things around in RestoreFromHistory to more closely match the non-fastback code path. Added comments to explain what part of the non-fastback code each section corresponds to.

Oops, fix the CanSavePresentation check to ignore the request we just added to the loadgroup. Note that this patch does not really handle the case where we fail for some reason to restore the presentation on the plevent. I'm still thinking about the best way to handle that -- I think I basically want to clear out the presentation on mLSHE and restart the load. But all of the parameters needed to do that will have to be cached on the plevent for that to work.

I'll take me a week or more to review this; I need to sort out the fastback code that I never reviewed when it landed in the process of reviewing this...

Restarting the load from the PLEvent as you suggest seems reasonable given that nsDocShell::Stop calls RevokeEvents.

Comment on attachment 190278 [details] [diff] [review] patch >Index: nsDocShell.cpp >+ event->mDocShell->RestoreFromHistory(); >+ // XXX need to fail over and do a normal load if something fails. How about capturing a result code from this function call, and then add a debug assertion to alert developers if this error case is being hit. That may help you to gather information about when that condition may occur. >+ nsresult rv = uiThreadQueue->PostEvent(evt); >+ if (NS_FAILED(rv)) >+ PL_DestroyEvent(evt); >+ >+ // The rest of the restore processing will happen on our PLEvent callback. >+ *aRestored = PR_TRUE; >+ return NS_OK; hrm... in the PostEvent failure case, we set aRestored to true. should we? maybe this parameter should be named aRestoring instead? >+ if (++gNumberOfDocumentsLoading == 1) >+ PL_FavorPerformanceHint(PR_TRUE, NS_EVENT_STARVATION_DELAY_HINT); >+ >+ nsCOMPtr<nsIDOMDocument> domDoc; >+ mContentViewer->GetDOMDocument(getter_AddRefs(domDoc)); >+ nsCOMPtr<nsIDocument> document = do_QueryInterface(domDoc); >+ if (document) { >+ SetCurrentURI(document->GetDocumentURI(), tweak indentation to be consistent r=darin

Comment on attachment 190278 [details] [diff] [review] patch requesting approval. I discussed this patch with Boris, and he said it would be ok if the patch landed with darin's review, then he can review later.

checked in

Comment on attachment 190278 [details] [diff] [review] patch Looks reasonable, though I filed bug 306283 on what looks like an issue with revoking the restore events....