The default bug view has changed. See this FAQ.

Ghost window on dirtymarkup.com

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Timvde, Assigned: khuey)

Tracking

Trunk
mozilla18
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2], URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 654814 [details]
about:memory report for dirtymarkup.com

about:memory reports two ghost windows on http://www.dirtymarkup.com/. This happens on an (almost┬╣) clean profile. Just opening and closing the page doesn't seem to create the ghost window. Clicking a few buttons/dropdowns does (even without running any of the scripts).

Relevant about:memory?verbose parts are attached. These are taken *after* closing the tab and clicking the "Minimize memory usage".

about:compartments also still lists http://dirtymarkup.com/ as a user compartment.

┬╣ The profile is "almost" clean in the way that I always use it to test things. It has no add-ons, but does have a few plug-ins (flash and Java) and some history.
Your report is for which Firefox version ?
Whiteboard: [memshrink]

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

5 years ago
Oh, right, when choosing "Advanced bug form", that isn't attached :)
I did select the appropriate version/platform; it's on 2012-08-23 Nightly (version 17), Linux x64.

Comment 3

5 years ago
about:cc shows a document in the CC graph after closing the tab.

Comment 4

5 years ago
So I see a runtime leak which gets released during shutdown.
The most suspicious release is the following, but perhaps the problem is elsewhere.
(I know nothing about how workers get destroyed.)
(I *think* #4 is reporting wrong class name)

0  nsDocument::Release (this=0xaf2e8400) at /home/smaug/mozilla/hg/mozilla/content/base/src/nsDocument.cpp:1711
#1  0xb4e555a3 in nsHTMLDocument::Release (this=0xaf2e8400) at /home/smaug/mozilla/hg/mozilla/content/html/document/src/nsHTMLDocument.cpp:239
#2  0xb459040d in nsCOMPtr_base::~nsCOMPtr_base (this=0x9fc01f00, __in_chrg=<optimized out>) at ../../dist/include/nsCOMPtr.h:408
#3  0xb45904c8 in nsCOMPtr<nsISupports>::~nsCOMPtr (this=0x9fc01f00, __in_chrg=<optimized out>) at ../../dist/include/nsCOMPtr.h:833
#4  0xb48b9813 in nsPresState::~nsPresState (this=0x9fc01f00, __in_chrg=<optimized out>) at /home/smaug/mozilla/hg/mozilla/layout/base/nsPresState.h:19
#5  0xb4fa76b9 in nsTArray<nsCOMPtr<nsISupports>, nsTArrayDefaultAllocator>::DestructRange (this=0x9fce9250, start=0, count=7) at ../../dist/include/nsTArray.h:1213
#6  0xb4fa6dce in nsTArray<nsCOMPtr<nsISupports>, nsTArrayDefaultAllocator>::RemoveElementsAt (this=0x9fce9250, start=0, count=7) at ../../dist/include/nsTArray.h:933
#7  0xb4fa5c1e in nsTArray<nsCOMPtr<nsISupports>, nsTArrayDefaultAllocator>::Clear (this=0x9fce9250) at ../../dist/include/nsTArray.h:944
#8  0xb50ae518 in (anonymous namespace)::MainThreadReleaseRunnable::Run (this=0x9fce9240) at /home/smaug/mozilla/hg/mozilla/dom/workers/WorkerPrivate.cpp:543
#9  0xb5cbff89 in nsThread::ProcessNextEvent (this=0xb7d35510, mayWait=true, result=0xbfffc8cf) at /home/smaug/mozilla/hg/mozilla/xpcom/threads/nsThread.cpp:624
#10 0xb5c5981d in NS_ProcessNextEvent_P (thread=0xb7d35510, mayWait=true) at /home/smaug/mozilla/hg/mozilla/ff_build/xpcom/build/nsThreadUtils.cpp:220
#11 0xb5cbf9a3 in nsThread::Shutdown (this=0xb2d7eb30) at /home/smaug/mozilla/hg/mozilla/xpcom/threads/nsThread.cpp:466
#12 0xb50a4dd7 in mozilla::dom::workers::RuntimeService::NoteIdleThread (this=0xaed61b60, aThread=0xb2d7eb30)
    at /home/smaug/mozilla/hg/mozilla/dom/workers/RuntimeService.cpp:1192
#13 0xb50ae948 in (anonymous namespace)::TopLevelWorkerFinishedRunnable::Run (this=0x9fce9f40) at /home/smaug/mozilla/hg/mozilla/dom/workers/WorkerPrivate.cpp:644
#14 0xb5cbff89 in nsThread::ProcessNextEvent (this=0xb7d35510, mayWait=true, result=0xbfffcadf) at /home/smaug/mozilla/hg/mozilla/xpcom/threads/nsThread.cpp:624
#15 0xb5c5981d in NS_ProcessNextEvent_P (thread=0xb7d35510, mayWait=true) at /home/smaug/mozilla/hg/mozilla/ff_build/xpcom/build/nsThreadUtils.cpp:220
#16 0xb50a449c in mozilla::dom::workers::RuntimeService::Cleanup (this=0xaed61b60) at /home/smaug/mozilla/hg/mozilla/dom/workers/RuntimeService.cpp:1023
#17 0xb50a5767 in mozilla::dom::workers::RuntimeService::Observe (this=0xaed61b60, aSubject=0x0, aTopic=0xb6ea224b "xpcom-shutdown-threads", aData=0x0)
    at /home/smaug/mozilla/hg/mozilla/dom/workers/RuntimeService.cpp:1244

Comment 5

5 years ago
nsGlobalWindow::FreeInnerObjects() is called on the nsGlobalWindow which holds pointer to
the leaked mDoc. Yet worker seems to release stuff only during shutdown.
I can't reproduce this.

Comment 7

5 years ago
Somehow http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?mark=1982-1984#1972 is causing this, I believe.
(Commenting it out fixes the leak.)
Still trying to understand all this code.
Whiteboard: [memshrink] → [MemShrink:P2]

Comment 8

5 years ago
We end up dispatching canceling status in NotifyPrivate, but somehow don't exit from the worker loop, 
which then means we don't call ScheduleDeletion.

Comment 9

5 years ago
I don't quite understand how the return value of ::Run should be handled.
We clearly return error code in some cases, but don't always check it, but do occasionally.
Created attachment 660037 [details] [diff] [review]
patch

Fixes the leak but there are other cases when the return value of Run() isn't
checked,.
Created attachment 660046 [details] [diff] [review]
patch

Let closing to succeed normally.

Ben, as you know, I'm not familiar with this code, so if you have better ideas
how to handle the situation, feel free to write a patch ;)

WorkerPrivate::NotifyInternal and WorkerPrivate::DoRunLoop just don't play
well with each others.
Assignee: nobody → bugs
Attachment #660037 - Attachment is obsolete: true
Attachment #660046 - Flags: review?(bent.mozilla)
Attachment #660046 - Flags: feedback?(amarchesini)
https://tbpl.mozilla.org/?tree=Try&rev=fe163a9022cf
And to be clear, the patch is especially for 
http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?rev=70a0eda09c3c&mark=3490-3491#3475

Updated

5 years ago
Attachment #660046 - Flags: review?(bent.mozilla)
Attachment #660046 - Flags: feedback?(amarchesini)
Created attachment 660780 [details]
minimal testcase
Created attachment 660814 [details] [diff] [review]
patch

This one then. Trying to keep the existing behavior, but still
ensure that we exit the event loop when mCloseHandlerFinished becomes true.
Attachment #660046 - Attachment is obsolete: true
Attachment #660814 - Flags: review?(bent.mozilla)
Attachment #660814 - Flags: feedback?(amarchesini)
Comment on attachment 660814 [details] [diff] [review]
patch

Review of attachment 660814 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +718,2 @@
>      JSObject* target = JS_GetGlobalObject(aCx);
> +    if (!target) {

I would like to read a comment here about why if !target, return true.
Attachment #660814 - Flags: feedback?(amarchesini) → feedback+
Comment on attachment 660814 [details] [diff] [review]
patch

khuey caught this in a recording and we looked it over today. This patch is close but we have a better one worked out.
Attachment #660814 - Flags: review?(bent.mozilla) → review-
Created attachment 667221 [details] [diff] [review]
Patch
Assignee: bugs → khuey
Attachment #660814 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #667221 - Flags: review?(bent.mozilla)
Comment on attachment 667221 [details] [diff] [review]
Patch

Review of attachment 667221 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +3494,5 @@
>      return true;
>    }
>  
> +  // If the worker script never ran, we don't need to do anything else, except
> +  // pretend that we ran the close handler.

Nit: This comment isn't totally right - needs to say something like "If the worker script never ran or failed to compile"

@@ +3507,1 @@
>    if (previousStatus == Running) {

Nit: We can combine these if blocks now right? E.g.:

  if (previousStatus == Running && aStatus != Killing) {
Attachment #667221 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] from comment #19)
> Comment on attachment 667221 [details] [diff] [review]
> Patch
> 
> Review of attachment 667221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +3494,5 @@
> >      return true;
> >    }
> >  
> > +  // If the worker script never ran, we don't need to do anything else, except
> > +  // pretend that we ran the close handler.
> 
> Nit: This comment isn't totally right - needs to say something like "If the
> worker script never ran or failed to compile"

If it fails to compile it never runs ...

> @@ +3507,1 @@
> >    if (previousStatus == Running) {
> 
> Nit: We can combine these if blocks now right? E.g.:
> 
>   if (previousStatus == Running && aStatus != Killing) {

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e04928c99aa
https://hg.mozilla.org/mozilla-central/rev/6e04928c99aa
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 667221 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Worker rewrite, long ago.
User impact if declined: Intermittent CC slowdowns that last until app shutdown.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): It's possible we've introduced a new race here but it is quite unlikely.
String or UUID changes made by this patch: None
Attachment #667221 - Flags: approval-mozilla-aurora?
(In reply to ben turner [:bent] from comment #22)
> Comment on attachment 667221 [details] [diff] [review]
> Patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Worker rewrite, long ago.
> User impact if declined: Intermittent CC slowdowns that last until app
> shutdown.

This isn't a new regression, and we don't have a lot of evidence pointing to the issue being prevalent. Do you have any data to the contrary?
(In reply to Alex Keybl [:akeybl] from comment #23)
> This isn't a new regression, and we don't have a lot of evidence pointing to
> the issue being prevalent. Do you have any data to the contrary?

I don't think we have ever had any reliable way of collecting that data... Maybe telemetry on slow CC times? Olli may have a better answer.
Even if telemetry may show some slower than expected CC times, it is not possible to
map those CC times to this bug.
But using workers is getting more common all the time...
Comment on attachment 667221 [details] [diff] [review]
Patch

Let's let this ride the trains given we think this carries non-zero risk and is a longstanding regression. There's only about 6 weeks till FF17's release, so it's time to start limiting risk.
Attachment #667221 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
I found that the minimal testcase comes with the ability to break web workers. When the web workers broke, the only way to use it again is to restart the browser :-/

http://people.mozilla.com/~tchien/worker-loop/

The number in this test case stops at 18.
You need to log in before you can comment on or make changes to this bug.