Closed Bug 580618 Opened 14 years ago Closed 14 years ago

GC removes HeadsUpDisplay after reloading the page

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: julian.viereck, Assigned: msucan)

References

Details

(Whiteboard: [patchclean:1116])

Attachments

(1 file, 5 obsolete files)

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.
(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?
btw: a new instance of HeadsUpDisplay and JSTerm are not created, only the HUDConsole object attached to the contentWindow
(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.
Attached patch Patch (obsolete) — Splinter Review
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)
Blocks: 578658
I cannot reproduce this bug
Comment on attachment 459008 [details] [diff] [review]
Patch

feedback r+ on Mihai's split up patches: bug 578658
Attachment #459008 - Flags: feedback?(ddahl) → feedback+
is this bug now obsolete in favor of bug 578658? If so, we should close this one.
As discussed with ddahl, we should concentrate on getting bug 578658 in that includes this patch/fixes this bug.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Since the patches in bug 578658 are not going to land, I think this bug should be reopened.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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).
Assignee: julian.viereck → pwalton
Severity: normal → blocker
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Whiteboard: [kd4b6]
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Blocks: devtools4b7
Attached patch switch to strong refs (obsolete) — Splinter Review
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
I would like to see a test case that proves the need for this patch. Show me a failing test.:)
needs a test and is not urgent at this point. moving to our b7 work.
Blocks: devtools4b8
No longer blocks: devtools4b7
Whiteboard: [kd4b6]
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)
Attached patch switch to strong refs (obsolete) — Splinter Review
Rebased patch, just for testing purposes. Requires the latest patch from bug 583476.
Attachment #472773 - Attachment is obsolete: true
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
No longer blocks: devtools4b8
Attached patch proposed patch (obsolete) — Splinter Review
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)
Blocks: 529086
Whiteboard: [patchclean:1112]
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+
(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
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)
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+
Attached patch updated patchSplinter Review
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)
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
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1116][checkin] → [patchclean:1116]
Target Milestone: --- → Firefox 4.0b8
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: