622315 - "ASSERTION: Entry added to loadgroup twice, don't do that"

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Entry added to loadgroup twice, don't do that: 'PL_DHASH_ENTRY_IS_FREE(entry)', file netwerk/base/src/nsLoadGroup.cpp, line 537

###!!! ASSERTION: should only have one RestorePresentationEvent: '!mRestorePresentationEvent.IsPending()', file docshell/base/nsDocShell.cpp, line 6862

Comment 1

[Jesse Ruderman]

Attachment: log with assertion stacks

Comment 2

[Jesse Ruderman]

Attachment: testcase that leaks

Adding a location.replace makes it leak in addition to asserting.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I can reproduce this on Windows.

We leak

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 944 bytes durin
g test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of n
sBaseChannel with size 156 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of n
sBaseURLParser with size 12 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of n
sDocShell::InterfaceRequestorProxy with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of n
sHashPropertyBag with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of n
sLoadGroup with size 8 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsPrincipal
with size 76 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsSimpleURI
with size 56 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsStandardU
RL with size 188 bytes each (376 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 6 instances of nsStringBuf
fer with size 8 bytes each (48 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsTArray_bas
e with size 4 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsVariant w
ith size 64 bytes each (128 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsWeakRefere
nce with size 16 bytes

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think I have a patch for this, testing it on try now.

This does raise the question of what

{
history.back();
history.back();
}

should actually do.  I've raised that as Bug 12382 on the spec.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This fixes the assertion/leak, and I don't think it breaks anything else.  It's still less than ideal though.  It goes through the motions of going back on the second history.back();, which is what the spec says to do but it also IMO wrong.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This appears to pass tests.  We return false from canGo[Back|Forward] when there's a pending navigation.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Updated Patch

bz, review ping?

Comment 8

[Boris Zbarsky [:bzbarsky]]

So with this patch, what happens if the user clicks on the back button twice?  How does that compare to what happens before the patch?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #8)
> So with this patch, what happens if the user clicks on the back button
> twice?  How does that compare to what happens before the patch?

I can't tell any difference in the behavior.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 531953 [details] [diff] [review]
Updated Patch

OK, r=me. Can you add a crashtest?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #10)
> Comment on attachment 531953 [details] [diff] [review] [review]
> Updated Patch
> 
> OK, r=me. Can you add a crashtest?

Not the provided testcase, since it uses window.open.  Might be able to beat it into a testable form though.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Why can't you use window.open?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #12)
> Why can't you use window.open?

The reftest suite (and hence crashtests) don't support window.open ... unless we've changed that recently.

I could check in the leaking version as a mochitest though.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/52b9e864be0e

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

I believe this patch is causing navigation to 'fail' in some cases on some sites.

Follow a link on say:
http://www.sevenforums.com/browsers-mail/

Note that at times the navigation buttons both grey out and clicking back does nothing.  

Possible STR
Follow any forum post
rather than clicking 'back' use the link at the top or bottom or the page and click on 'browsers & mail' should return to forum page with links marked as 'read' 
Its at this point I think the navigation arrows grey out, and following subsequent links will result in no way to use back/forward navigation. 

Broken in cset: 
http://hg.mozilla.org/mozilla-central/rev/52b9e864be0e
Works ok in cset:
http://hg.mozilla.org/mozilla-central/rev/93911949517c

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I can't reproduce, but it's certainly possible this is to blame.  A testcase would really help here ...

Comment 17

[Jim Jeffery not reading bug-mail 1/2/11]

Also experiencing this at:
http://forums.mozillazine.org/viewforum.php?f=23

Opening the forum in a 'new tab' and closing the old tab seems to correct the problem for awhile.

Comment 18

[Jim Jeffery not reading bug-mail 1/2/11]

Maybe this is better:

1. visit: http://www.sevenforums.com/browsers-mail/
2. follow any forum post link
3. click 'back' or use your mouse back-button or left arrow
4. refresh the page with right-click 'reload' 
5. Now note: that the navigation arrows grey out and all following attempts to follow a post and navigate back fail.

Opening the site in a new tab restores navigation until you do step 4 above.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That I can reproduce.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Filed Bug 662200.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The cause didn't jump out at me, so I backed this out.

http://hg.mozilla.org/mozilla-central/rev/068197c2a88e

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #21)
> The cause didn't jump out at me, so I backed this out.
> 
> http://hg.mozilla.org/mozilla-central/rev/068197c2a88e

Just confirming using the latest hourly that the backout indeed fixed the problem, hope you track it down without too much trouble.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> The reftest suite (and hence crashtests) don't support window.open

Oh, they get popup-blocked?  If so, and if window.open would help, then by all means mochitest your way to victory!

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/projects/cedar/rev/ffead16f25eb

Comment 25

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ffead16f25eb

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This was backed out of Aurora to fix Bug 670318.

http://hg.mozilla.org/releases/mozilla-aurora/rev/00ceaca88670

Comment 27

[Justin Lebar (not reading bugmail)]

Attachment: Backout

Comment 28

[Justin Lebar (not reading bugmail)]

Backed out of trunk per bug 690408 comment 8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6405a1d4b22

This patch is in current Aurora (FF 9) and Beta (FF 8).  I think we want to back it out from both.

Comment 29

[Justin Lebar (not reading bugmail)]

Release drivers: This backout fixes bug 682115 and may fix bug 683886.

Comment 30

[Matt Brubeck (:mbrubeck)]

Backout landed on mozilla-central for Firefox 10:
https://hg.mozilla.org/mozilla-central/rev/c6405a1d4b22

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

status-firefox10: --- ➔ affected
status-firefox9: --- ➔ fixed
status-firefox8: --- ➔ fixed

Is this correct? I don't see a backout for beta, but one for central

Comment 32

[Justin Lebar (not reading bugmail)]

(In reply to Ms2ger from comment #31)
> status-firefox10: --- ➔ affected
> status-firefox9: --- ➔ fixed
> status-firefox8: --- ➔ fixed
> 
> Is this correct? I don't see a backout for beta, but one for central

Correct.  He's indicating that this bug affects FF10, since it's backed out there, but is fixed on FF9 and FF8, since it hasn't been backed out there (yet).

Comment 33

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> (In reply to comment #8)
> > So with this patch, what happens if the user clicks on the back button
> > twice?  How does that compare to what happens before the patch?
> 
> I can't tell any difference in the behavior.

With this patch, if I click back while the throbber is going, I don't go back.

Comment 34

[Justin Lebar (not reading bugmail)]

/me throws up his hands.

G+ (bug 682115) is not fixed with this backout.  Forward works sometimes now, but not always.  (I didn't think to test multiple times when I tested the backout.)

The failing behavior is slightly different with the backout.  At least sometimes, when you go forward, the throbber briefly throbs.  The forward button is still stuck in this case, though.

It could be that we're doing everything correctly now and G+ simply traps the forward button.  But I'm now unsure whether we want to take this on branches.

Comment 35

[Justin Lebar (not reading bugmail)]

(The reason to still take on branches would be the suspicion that this fixes bug 683886.)

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should still take this on branches because it does fix some known intermittent breakage (I've dropped into a debugger before and seen that this was an issue, just have never been able to reproduce the steps to get to that situation).

Comment 37

[Justin Lebar (not reading bugmail)]

To be clear, you mean we should still take the backout on branches, right?

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes.

Comment 39

[Christopher Blizzard (:blizzard)]

There's an approval request here but it sounds like you guys aren't sure if it fixes problems or not?  We need more info here from the triage folks to make a decision here.

Comment 40

[Justin Lebar (not reading bugmail)]

(In reply to Christopher Blizzard (:blizzard) from comment #39)
> There's an approval request here but it sounds like you guys aren't sure if
> it fixes problems or not?  We need more info here from the triage folks to
> make a decision here.

It's pretty hard to tell whether the backout fixes bug 683886 or khuey's intermittent breakage (comment 36), since they happen only occasionally and don't have a reliable str.  I'm not sure how to get you more info, either.

The backout doesn't fix G+ from the user's perspective, so I don't think it's worth taking the backout on branches for the sake of that site.

Comment 41

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Even if this doesn't fix the bug we wanted to back it out to fix, we should back it out anyways.  We backed it out of Aurora a few cycles ago because there were some problems with it, and we haven't managed to track them all down yet (and now believe that there's a fundamental problem with the patch (comment 33)).

Comment 42

[Justin Lebar (not reading bugmail)]

Thanks, release drivers.  Backed out of Aurora and Beta.

https://hg.mozilla.org/releases/mozilla-aurora/rev/516f06fad5cf
https://hg.mozilla.org/releases/mozilla-beta/rev/b49b1ea08d3c

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm no longer the right owner for htis.