Bug 40926 - (Blocked by SVG) high and unfreed memory usage on script tag insertion
Summary: (Blocked by SVG) high and unfreed memory usage on script tag insertion
Status: RESOLVED DUPLICATE of bug 59604
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Major
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-06-21 10:13 PDT by Alexander Romanovich
Modified: 2011-04-29 15:19 PDT (History)
9 users (show)

See Also:


Attachments
test case for bug (432 bytes, text/html)
2010-06-21 10:13 PDT, Alexander Romanovich
no flags Details
proposed fix (19.94 KB, patch)
2010-12-14 17:18 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
work in progress, breaks SVG (20.73 KB, patch)
2010-12-15 11:05 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
SVG no subresources test (152 bytes, image/svg+xml)
2010-12-15 11:07 PST, Alexey Proskuryakov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Romanovich 2010-06-21 10:13:19 PDT
I've attached a script that will append and remove a script tag to a div over the course of 10 seconds.

When you run it in Safari 5.0 with the Activity Monitor open, you will observe the memory usage of Safari climb steadily for the 10 seconds the script runs for. Repeat clicks will very quickly drive memory usage up over 1gig.

If you take the same code and run it in Firefox 3.6.3, you will also see the memory usage go up but: 1) by a much much smaller amount 2) over repeat clicks Firefox's memory usage will suddenly drop back down, and will not continue to climb endlessly.

This problem exists also in WebKit nightly (61502).
Comment 1 Alexander Romanovich 2010-06-21 10:13:46 PDT
Created attachment 59262 [details]
test case for bug
Comment 2 Alexey Proskuryakov 2010-06-22 12:29:09 PDT
These script elements aren't garbage collected, there were over 125000 left alive after the test finished for me.
Comment 3 Alexey Proskuryakov 2010-06-23 17:00:35 PDT
<rdar://problem/8124718>
Comment 4 Alexey Proskuryakov 2010-12-14 14:25:14 PST
This is caused by this code in JSDOMBinding.h:

        // If a wrapper is the last reference to an image or script element
        // that is loading but not in the document, the wrapper is observable
        // because it is the only thing keeping the image element alive, and if
        // the image element is destroyed, its load event will not fire.
        // FIXME: The DOM should manage this issue without the help of JavaScript wrappers.
        if (node->hasTagName(imgTag) && !static_cast<HTMLImageElement*>(node)->haveFiredLoadEvent())
            return true;
        if (node->hasTagName(scriptTag) && !static_cast<HTMLScriptElement*>(node)->haveFiredLoadEvent())
            return true;
#if ENABLE(VIDEO)
        if (node->hasTagName(audioTag) && !static_cast<HTMLAudioElement*>(node)->paused())
            return true;
#endif
Comment 5 Alexey Proskuryakov 2010-12-14 14:25:47 PST
Clearly, haveFiredLoadEvent() is not the same as "is loading".
Comment 6 Alexey Proskuryakov 2010-12-14 14:53:40 PST
Besides this synthetic test case, this affects inline scripts, because they also don't fire the load event. Script elements with src are probably affected if load fails...
Comment 7 Alexey Proskuryakov 2010-12-14 17:18:39 PST
Created attachment 76598 [details]
proposed fix

It seems the we may get this flag wrong if src attribute is removed or changed during loading. Fixing that is something for another day.

See also: bug 39628, bug 31253, bug 23372.
Comment 8 WebKit Review Bot 2010-12-14 17:26:15 PST
Attachment 76598 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7096004
Comment 9 Early Warning System Bot 2010-12-14 17:36:53 PST
Attachment 76598 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7160006
Comment 10 Eric Seidel (no email) 2010-12-15 04:05:21 PST
Attachment 76598 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7158026
Comment 11 Alexey Proskuryakov 2010-12-15 10:44:37 PST
Comment on attachment 76598 [details]
proposed fix

SVG tests are failing, need to fix.
Comment 12 Alexey Proskuryakov 2010-12-15 11:05:53 PST
Created attachment 76669 [details]
work in progress, breaks SVG

SVG seems quite complicated - when I remove the failing assertion, svg/dom/SVGScriptElement/script-load-and-error-events.svg breaks badly. I don't know the SVG spec and can't tell for sure if its expected results are correct.

I suspect that SVG is not quite right:
1) The test expects onload for <script xlink:href="resources/certainlydoesnotexist.js"></script>
2) WebKit fires onload for <script> without any external resources.
Comment 13 Alexey Proskuryakov 2010-12-15 11:07:39 PST
Created attachment 76670 [details]
SVG no subresources test
Comment 14 Alexander Romanovich 2011-01-25 06:43:39 PST
Is anyone working on the SVG block for this bug? Maybe a separate bug filed for it that I can CC myself on?
Comment 15 Eric Seidel (no email) 2011-01-25 14:37:32 PST
(In reply to comment #12)
> Created an attachment (id=76669) [details]
> work in progress, breaks SVG
> 
> SVG seems quite complicated - when I remove the failing assertion, svg/dom/SVGScriptElement/script-load-and-error-events.svg breaks badly. I don't know the SVG spec and can't tell for sure if its expected results are correct.
> 
> I suspect that SVG is not quite right:
> 1) The test expects onload for <script xlink:href="resources/certainlydoesnotexist.js"></script>
> 2) WebKit fires onload for <script> without any external resources.

I haven't looked at the test in quesiton.  However SVG has a separate load event "SVGLoad":
http://www.w3.org/TR/2002/PR-SVG11-20021115/interact.html

It's fired basically every time a tag is closed if there is a listener.
Comment 16 Geoffrey Garen 2011-04-29 15:09:40 PDT
It looks like the test case here is fixed by the fix for bug 59604.

*** This bug has been marked as a duplicate of bug 59604 ***
Comment 17 Alexey Proskuryakov 2011-04-29 15:19:43 PDT
It's certainly correct to close this as a dupe, but we'll need to sort out the other bugs and issues touched by this patch eventually.