631440 - Crash [@ nsDocShell::AddToSessionHistory] with pushState

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Based on when I started seeing this bug, I suspect it's a regression from bug 602256.

Like in bug 630801, the pushState call has an effect despite throwing.

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Dave Townsend [:mossop]]

Silly question, how do I reproduce this? I loaded the testcase in a debug build and nothing happened except the popup blocker showed

Comment 3

[Dave Townsend [:mossop]]

Guess I can see the problem though, attempting to pushstate before there is an initial history state will crash. Not sure what the behaviour should be there (probably throw an exception) but an easy fix is to just not clone in that case.

Comment 4

[Dave Townsend [:mossop]]

Should have looked closer at the stack. The call to pushstate happens before the load state but does throw an exception, however before that it sets mLoadType to LOAD_PUSHSTATE. The subsequent load of the window then acts somewhat as if it was a pushState and so attempts to clone the history. Simple fix is to move setting mLoadType to after all the checks are performed and we actually start to mutate things. This might fix bug 630801 too, pushing to try now.

Comment 5

[Jesse Ruderman]

This testcase requires popups to be enabled.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This moves setting mLoadType to later and that alone is enough to resolve the crash. I also added a check on mOSHE to the specific code that changed for protection. The testcase successfully crashes without the fix applied and passes with.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 510306 [details] [diff] [review]
patch rev 1

r=me

Comment 8

[Dave Townsend [:mossop]]

Landed tis yesterday: http://hg.mozilla.org/mozilla-central/rev/33dc58befe48