The default bug view has changed. See this FAQ.

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

NEW
Unassigned

Status

()

Core
Document Navigation
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, mlk, testcase})

Trunk
assertion, mlk, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox8 unaffected, firefox9 unaffected, firefox10 affected)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 500571 [details]
log with assertion stacks
(Reporter)

Comment 2

6 years ago
Created attachment 522142 [details]
testcase that leaks

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

Updated

6 years ago
Keywords: mlk
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
OS: Mac OS X → All
Hardware: x86 → DEC
Hardware: DEC → All

Updated

6 years ago
Hardware: All → DEC
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.
Assignee: nobody → khuey
Hardware: DEC → All
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.
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.
Attachment #522160 - Attachment is obsolete: true
Attachment #524669 - Flags: review?(bzbarsky)
Created attachment 531953 [details] [diff] [review]
Updated Patch

bz, review ping?
Attachment #524669 - Attachment is obsolete: true
Attachment #531953 - Flags: review?(bzbarsky)
Attachment #524669 - Flags: review?(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?
(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 on attachment 531953 [details] [diff] [review]
Updated Patch

OK, r=me. Can you add a crashtest?
Attachment #531953 - Flags: review?(bzbarsky) → review+
(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.
Why can't you use window.open?
(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.
http://hg.mozilla.org/mozilla-central/rev/52b9e864be0e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
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
I can't reproduce, but it's certainly possible this is to blame.  A testcase would really help here ...
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.
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.
That I can reproduce.
Depends on: 662200
Filed Bug 662200.
The cause didn't jump out at me, so I backed this out.

http://hg.mozilla.org/mozilla-central/rev/068197c2a88e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
> 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!
http://hg.mozilla.org/projects/cedar/rev/ffead16f25eb
Whiteboard: fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/ffead16f25eb
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Depends on: 665964
This was backed out of Aurora to fix Bug 670318.

http://hg.mozilla.org/releases/mozilla-aurora/rev/00ceaca88670
Target Milestone: mozilla7 → mozilla8
Created attachment 564342 [details] [diff] [review]
Backout
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.
Attachment #564342 - Flags: approval-mozilla-beta?
Attachment #564342 - Flags: approval-mozilla-aurora?
Release drivers: This backout fixes bug 682115 and may fix bug 683886.
(Reporter)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout landed on mozilla-central for Firefox 10:
https://hg.mozilla.org/mozilla-central/rev/c6405a1d4b22
status-firefox10: --- → affected
status-firefox8: --- → fixed
status-firefox9: --- → fixed
status-firefox10: --- ➔ affected
status-firefox9: --- ➔ fixed
status-firefox8: --- ➔ fixed

Is this correct? I don't see a backout for beta, but one for central
(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).
(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.
Attachment #564342 - Flags: approval-mozilla-beta?
Attachment #564342 - Flags: approval-mozilla-beta+
Attachment #564342 - Flags: approval-mozilla-aurora?
Attachment #564342 - Flags: approval-mozilla-aurora+
/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.
(The reason to still take on branches would be the suspicion that this fixes bug 683886.)
Attachment #564342 - Flags: approval-mozilla-beta?
Attachment #564342 - Flags: approval-mozilla-beta+
Attachment #564342 - Flags: approval-mozilla-aurora?
Attachment #564342 - Flags: approval-mozilla-aurora+
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).
To be clear, you mean we should still take the backout on branches, right?
Yes.
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.
(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.
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)).

Updated

6 years ago
Attachment #564342 - Flags: approval-mozilla-beta?
Attachment #564342 - Flags: approval-mozilla-beta+
Attachment #564342 - Flags: approval-mozilla-aurora?
Attachment #564342 - Flags: approval-mozilla-aurora+
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
status-firefox8: fixed → affected
status-firefox9: fixed → affected

Updated

6 years ago
status-firefox8: affected → unaffected
status-firefox9: affected → unaffected
I'm no longer the right owner for htis.
Assignee: khuey → nobody
Status: REOPENED → NEW
Target Milestone: mozilla8 → ---
Depends on: 715856
You need to log in before you can comment on or make changes to this bug.