Last Comment Bug 622315 - "ASSERTION: Entry added to loadgroup twice, don't do that"
: "ASSERTION: Entry added to loadgroup twice, don't do that"
Status: NEW
: assertion, mlk, testcase
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 662200 665964 715856
Blocks: 594645
  Show dependency treegraph
 
Reported: 2010-12-31 21:47 PST by Jesse Ruderman
Modified: 2012-01-06 16:04 PST (History)
12 users (show)
khuey: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
affected


Attachments
testcase (511 bytes, text/html)
2010-12-31 21:47 PST, Jesse Ruderman
no flags Details
log with assertion stacks (30.01 KB, text/plain)
2010-12-31 21:47 PST, Jesse Ruderman
no flags Details
testcase that leaks (554 bytes, text/html)
2011-03-26 14:56 PDT, Jesse Ruderman
no flags Details
Patch (864 bytes, patch)
2011-03-26 16:59 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (1.33 KB, patch)
2011-04-08 10:37 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Updated Patch (1.33 KB, patch)
2011-05-12 09:33 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bzbarsky: review+
Details | Diff | Splinter Review
Backout (1.56 KB, patch)
2011-10-03 14:43 PDT, Justin Lebar (not reading bugmail)
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-12-31 21:47:18 PST
Created attachment 500570 [details]
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 2010-12-31 21:47:43 PST
Created attachment 500571 [details]
log with assertion stacks
Comment 2 Jesse Ruderman 2011-03-26 14:56:02 PDT
Created attachment 522142 [details]
testcase that leaks

Adding a location.replace makes it leak in addition to asserting.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-26 15:22:56 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-26 16:41:03 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-26 16:59:29 PDT
Created attachment 522160 [details] [diff] [review]
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-08 10:37:56 PDT
Created attachment 524669 [details] [diff] [review]
Patch

This appears to pass tests.  We return false from canGo[Back|Forward] when there's a pending navigation.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-12 09:33:39 PDT
Created attachment 531953 [details] [diff] [review]
Updated Patch

bz, review ping?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-05-12 19:36:25 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-27 16:54:28 PDT
(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 [:bz] (still a bit busy) 2011-05-27 19:28:31 PDT
Comment on attachment 531953 [details] [diff] [review]
Updated Patch

OK, r=me. Can you add a crashtest?
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-30 14:25:44 PDT
(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 [:bz] (still a bit busy) 2011-05-30 17:43:57 PDT
Why can't you use window.open?
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-03 15:21:11 PDT
(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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-05 12:13:23 PDT
http://hg.mozilla.org/mozilla-central/rev/52b9e864be0e
Comment 15 Jim Jeffery not reading bug-mail 1/2/11 2011-06-05 15:51:20 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-05 15:57:27 PDT
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 2011-06-05 15:59:09 PDT
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 2011-06-05 16:05:36 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-05 16:06:57 PDT
That I can reproduce.
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-05 16:10:09 PDT
Filed Bug 662200.
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-05 16:23:27 PDT
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 2011-06-05 18:39:57 PDT
(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 [:bz] (still a bit busy) 2011-06-07 01:13:17 PDT
> 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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-07 13:26:19 PDT
http://hg.mozilla.org/projects/cedar/rev/ffead16f25eb
Comment 26 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-10 06:54:53 PDT
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) 2011-10-03 14:43:42 PDT
Created attachment 564342 [details] [diff] [review]
Backout
Comment 28 Justin Lebar (not reading bugmail) 2011-10-03 14:48:13 PDT
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) 2011-10-03 14:50:31 PDT
Release drivers: This backout fixes bug 682115 and may fix bug 683886.
Comment 30 Matt Brubeck (:mbrubeck) 2011-10-03 16:46:22 PDT
Backout landed on mozilla-central for Firefox 10:
https://hg.mozilla.org/mozilla-central/rev/c6405a1d4b22
Comment 31 :Ms2ger (⌚ UTC+1/+2) 2011-10-03 23:24:05 PDT
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) 2011-10-04 05:37:58 PDT
(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) 2011-10-04 07:00:47 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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) 2011-10-04 18:43:48 PDT
/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) 2011-10-04 18:44:26 PDT
(The reason to still take on branches would be the suspicion that this fixes bug 683886.)
Comment 36 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-05 18:42:06 PDT
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) 2011-10-05 18:57:43 PDT
To be clear, you mean we should still take the backout on branches, right?
Comment 38 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-05 19:02:29 PDT
Yes.
Comment 39 Christopher Blizzard (:blizzard) 2011-10-06 14:23:02 PDT
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) 2011-10-06 14:33:36 PDT
(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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-07 04:30:11 PDT
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) 2011-10-10 16:30:53 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-19 11:06:42 PDT
I'm no longer the right owner for htis.

Note You need to log in before you can comment on or make changes to this bug.