GC removes HeadsUpDisplay after reloading the page

RESOLVED FIXED in Firefox 4.0b8

Status

()

Firefox
Developer Tools
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Julian Viereck, Assigned: msucan)

Tracking

unspecified
Firefox 4.0b8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [patchclean:1116])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 years ago
Currently, when the page is reloaded and the WebConsole is activated, a new instance of a HeadsUpDisplay and JSTerm is created. If you try to reference the HeadsUpDisplay after a short period of time (5 - 10sec) using the weakReference and the hudId, the HeadsUpDisplay object is no longer there. It looks to me like the GC removed the HeadsUpDisplay that is referenced to by the weakReference binding.

Comment 1

8 years ago
(In reply to comment #0)
> Currently, when the page is reloaded and the WebConsole is activated, a new
> instance of a HeadsUpDisplay and JSTerm is created. If you try to reference the
> HeadsUpDisplay after a short period of time (5 - 10sec) using the weakReference
> and the hudId, the HeadsUpDisplay object is no longer there. It looks to me
> like the GC removed the HeadsUpDisplay that is referenced to by the
> weakReference binding.

"reference" how? in test code? by entering things into the console?

Comment 2

8 years ago
btw: a new instance of HeadsUpDisplay and JSTerm are not created, only the HUDConsole object attached to the contentWindow
(Reporter)

Comment 3

8 years ago
(In reply to comment #1)
> (In reply to comment #0)
> > Currently, when the page is reloaded and the WebConsole is activated, a new
> > instance of a HeadsUpDisplay and JSTerm is created. If you try to reference the
> > HeadsUpDisplay after a short period of time (5 - 10sec) using the weakReference
> > and the hudId, the HeadsUpDisplay object is no longer there. It looks to me
> > like the GC removed the HeadsUpDisplay that is referenced to by the
> > weakReference binding.
> 
> "reference" how? in test code? by entering things into the console?

This issue gets visible, when applying the patches of bug 578658. Sending messages to a HeadsUpDisplay stops working after reloading the page while the WebConsole is opened with a delay of 6-10 seconds. I've got a test page to reproduce the problem.

(In reply to comment #2)
> btw: a new instance of HeadsUpDisplay and JSTerm are not created, only the
> HUDConsole object attached to the contentWindow

I've added some log messages when a new HeadsUpDisplay is created. Whenever the page is reloaded a new HeadsUpDisplay is created.
(Reporter)

Comment 4

8 years ago
Created attachment 459008 [details] [diff] [review]
Patch

After applying this patch, when the page is reloaded, there is no new HeadsUpDisplay and JSTerm object created anymore. Instead, only the console object from the already existing HeadsUpDisplay object is set to the window object. This solves the problem that the GC removes the HeadsUpDisplay.

This patch also removes the function HUD_reattachConsole and some other code, that is no longer needed anymore due to the way the console is attached to the window now.
Attachment #459008 - Flags: feedback?(ddahl)
(Reporter)

Updated

8 years ago
Blocks: 578658

Comment 5

8 years ago
I cannot reproduce this bug

Comment 6

8 years ago
Comment on attachment 459008 [details] [diff] [review]
Patch

feedback r+ on Mihai's split up patches: bug 578658
Attachment #459008 - Flags: feedback?(ddahl) → feedback+

Comment 7

8 years ago
is this bug now obsolete in favor of bug 578658? If so, we should close this one.
(Reporter)

Comment 8

8 years ago
As discussed with ddahl, we should concentrate on getting bug 578658 in that includes this patch/fixes this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 578658
Since the patches in bug 578658 are not going to land, I think this bug should be reopened.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Comment 10

7 years ago
From looking over the patch attached to this bug, it might get fixed by applying the patch from bug 583476 (WebConsole fails to activate when window.console is already defined).

Updated

7 years ago
Assignee: julian.viereck → pwalton

Updated

7 years ago
Severity: normal → blocker

Comment 11

7 years ago
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Whiteboard: [kd4b6]

Comment 12

7 years ago
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal

Updated

7 years ago
Blocks: 593957
(Assignee)

Comment 13

7 years ago
Created attachment 472773 [details] [diff] [review]
switch to strong refs

This patch switches the HUDService and test code to use strong HeadsUpDisplay object references, instead of weakrefs that are sometimes GC'ed too early.

This patch requires attachment 472728 [details] [diff] [review]. That's required because it changes the windowInitializer() to no longer create new instances for existing HUD objects.
Assignee: pwalton → mihai.sucan

Comment 14

7 years ago
I would like to see a test case that proves the need for this patch. Show me a failing test.:)

Comment 15

7 years ago
needs a test and is not urgent at this point. moving to our b7 work.
Blocks: 594225
No longer blocks: 593957
Whiteboard: [kd4b6]
(Assignee)

Comment 16

7 years ago
Created attachment 474411 [details] [diff] [review]
mochitest for GC of HUD weak refs

Patch that only adds a test file to show the GC issue exists with HUD weak refs. Applies cleanly on mozilla-central.

What the test does is only load a page, get the hudId and reload the page, and use that to get the hud weak ref. After page reload the HUD weak ref is garbage collected.

Please test and confirm. Thanks!
Attachment #474411 - Flags: feedback?(ddahl)
(Assignee)

Comment 17

7 years ago
Created attachment 474535 [details] [diff] [review]
switch to strong refs

Rebased patch, just for testing purposes. Requires the latest patch from bug 583476.
Attachment #472773 - Attachment is obsolete: true
(Assignee)

Comment 18

7 years ago
More information based on what I did yesterday:

- attachment 474411 [details] [diff] [review] allows us to test if we have problems with the HUD weak refs after GC.

- the patch from bug 583476 changes some stuff in the windowInitializer() method such that HUD weak refs do not get lost on page reload. Debug builds show no memleaks.

- attachment 474535 [details] [diff] [review] switches the code to use strong refs. Debug builds show no memleaks.


Thoughts:

- if the patch from bug 583476 is checked-in ... we'll have a nice-enough fix for the GC issues for HUD weak refs.

Still, that will only continue to work as long as other code contributors do not make subtle changes that break the very thin balance that we have. How it works: we store a weak ref for each HUD, and a strong ref inside the HUDConsole instance that is inside the HUD object, which in turn becomes the public window.console API, and thus we have the HUD object at all times - the GC process won't clear it.

However, I've seen code evolution that do not keeps this nice balance - the HUD strong ref from the HUDConsole object may or may not be removed in the future.

- if we switch to strong refs, we will no longer have this situation. This change may be too late for Firefox 4.

For the Fx4 timeframe I think we should only push the patch from bug 583476 and the mochitest code from this bug report. After that, we should probably go for strong refs, and no longer use any weak refs.
OS: Mac OS X → All
Hardware: x86 → All

Updated

7 years ago
No longer blocks: 594225
(Assignee)

Comment 19

7 years ago
Created attachment 490135 [details] [diff] [review]
proposed patch

Proposed patch. This is basically a rebase of attachment 474535 [details] [diff] [review].

Resurrecting this patch due to bug 605621 - Patrick bumped once again into the problem of disappearing hudWeakRefs (thanks to GC). In yesterday's meeting we decided I am to rebase this patch.

Looking forward to your feedback! The patch should be easy to review - trivial changes. Thanks!
Attachment #459008 - Attachment is obsolete: true
Attachment #474411 - Attachment is obsolete: true
Attachment #474535 - Attachment is obsolete: true
Attachment #490135 - Flags: feedback?(ddahl)
Attachment #474411 - Flags: feedback?(ddahl)
(Assignee)

Updated

7 years ago
Blocks: 529086
Whiteboard: [patchclean:1112]

Comment 20

7 years ago
Comment on attachment 490135 [details] [diff] [review]
proposed patch

Is there a manual process to test that this fixes the GC problem? A procedure that QA can perform in their testing? Is that important?
Attachment #490135 - Flags: feedback?(ddahl) → feedback+
(Assignee)

Comment 21

7 years ago
(In reply to comment #20)
> Comment on attachment 490135 [details] [diff] [review]
> proposed patch
> 
> Is there a manual process to test that this fixes the GC problem? A procedure
> that QA can perform in their testing? Is that important?

Not really. With this patch applied we no longer have any weak refs that can go through the GC process. We can no longer loose refs to HUD objects suddenly.

Thanks for the feedback+!
Blocks: 605621
(Assignee)

Comment 22

7 years ago
Comment on attachment 490135 [details] [diff] [review]
proposed patch

Asking for review from Gavin. This patch fixes a longstanding issue with the HUDService: from time to time it looses the hudWeakReferences to various HUD objects. This bug has affected us at various points in time. Now it affects the patch from bug 605621.

We need this patch to finally get away from weak refs.
Attachment #490135 - Flags: review?(gavin.sharp)
(Assignee)

Comment 23

7 years ago
Asking for blocking2.0+: this patch solves a longstanding issue with the Web Console code, which prevents us from making use of hudWeakReferences. There are other patches (eg. bug 605621) that need to provide improved user functionality. The cost of properly implementing those would be lower if we switch away from hudWeakRefs - fewer random bugs/failures.

Thanks!
blocking2.0: --- → ?
(In reply to comment #23)
> Asking for blocking2.0+: this patch solves a longstanding issue with the Web
> Console code, which prevents us from making use of hudWeakReferences. There are
> other patches (eg. bug 605621) that need to provide improved user
> functionality. The cost of properly implementing those would be lower if we
> switch away from hudWeakRefs - fewer random bugs/failures.
> 
> Thanks!

To add to this: the user-facing impact of this is that the Web Console can suddenly mysteriously stop working at any time. When the GC decides to remove the weak reference, new messages will not be delivered, and the console cannot be interacted with. Hence this bug is critical, IMHO.
+1 to this. I've never liked using weakRefs for our console object.

Looks good, mihai.
Blocks: 592469
blocking2.0: ? → betaN+
Created attachment 491059 [details] [diff] [review]
updated patch

I took the liberty of updating this, it'll be nice to get this landed.
Attachment #490135 - Attachment is obsolete: true
Attachment #491059 - Flags: review+
Attachment #490135 - Flags: review?(gavin.sharp)
(Assignee)

Comment 27

7 years ago
Thanks for the review+ Gavin, and for the patch update!
Whiteboard: [patchclean:1112] → [patchclean:1116][checkin]
https://hg.mozilla.org/mozilla-central/rev/0281473dbd79
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1116][checkin] → [patchclean:1116]
Target Milestone: --- → Firefox 4.0b8
(Assignee)

Updated

7 years ago
Duplicate of this bug: 592222
You need to log in before you can comment on or make changes to this bug.