Regression: URL.revokeObjectURL() stopped working on 2016-07-17.

RESOLVED FIXED in Firefox 50

Status

()

--
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jujjyl, Assigned: amarchesini)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox50+ fixed, firefox51+ fixed, firefox52+ fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
It looks like since commit https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d8201a97184f2f6cdcd70f19d8d6fc90e5589f33&tochange=a1b20019c22d1a1f177387c754ec2eed74aced1b, URL.revokeObjectURL() stopped from actually releasing the memory, but it is being held on in about:memory.

STR:

1. Download and unzip attachment and launch it e.g. in "python -m SimpleHTTPServer"
2. Wait until the blue screen loads up (with the text "trial" at bottom right)
3. Open about:memory and tap the GC/CC/Minimize memory usage (for good measure), and then capture a snapshot.

Observed:

Before the commit from bug https://bugzilla.mozilla.org/show_bug.cgi?id=1282407, the snapshot reads

403.00 MB (100.0%) -- explicit
├──297.50 MB (73.82%) -- window-objects/top(http://localhost:8001/, id=2147483651)
│  ├──297.13 MB (73.73%) -- active/window(http://localhost:8001/)
│  │  ├──296.86 MB (73.66%) -- js-compartment(http://localhost:8001/)
│  │  │  ├──296.46 MB (73.56%) -- classes
│  │  │  │  ├──260.36 MB (64.61%) -- class(ArrayBuffer)/objects
│  │  │  │  │  ├──256.00 MB (63.52%) ── non-heap/elements/asm.js
│  │  │  │  │  ├────4.36 MB (01.08%) ── malloc-heap/elements/normal
│  │  │  │  │  └────0.00 MB (00.00%) ── gc-heap
│  │  │  │  ├───34.94 MB (08.67%) -- class(WebAssembly.Instance)/objects
│  │  │  │  │   ├──23.02 MB (05.71%) ── non-heap/code/asm.js
│  │  │  │  │   ├──11.93 MB (02.96%) -- malloc-heap
│  │  │  │  │   │  ├──11.93 MB (02.96%) ── misc
│  │  │  │  │   │  └───0.00 MB (00.00%) ── slots
│  │  │  │  │   └───0.00 MB (00.00%) ── gc-heap
│  │  │  │  └────1.15 MB (00.29%) ++ (7 tiny)
│  │  │  └────0.40 MB (00.10%) ++ (6 tiny)
│  │  └────0.27 MB (00.07%) ++ (4 tiny)
│  └────0.37 MB (00.09%) ++ js-zone(0x12aa8d800)
├───62.43 MB (15.49%) -- js-non-window
│   ├──50.83 MB (12.61%) -- runtime
│   │  ├──46.78 MB (11.61%) ── shared-immutable-strings-cache
│   │  └───4.05 MB (01.01%) ++ (11 tiny)
│   ├──10.36 MB (02.57%) -- zones
│   │  ├───9.00 MB (02.23%) ++ zone(0x119a32800)
│   │  └───1.36 MB (00.34%) ++ (2 tiny)
│   └───1.25 MB (00.31%) ++ gc-heap
├───34.08 MB (08.46%) ── heap-unclassified
├────4.96 MB (01.23%) ++ heap-overhead
└────4.02 MB (01.00%) ++ (18 tiny)

after the commit, it says

434.23 MB (100.0%) -- explicit
├──297.50 MB (68.51%) -- window-objects/top(http://localhost:8001/, id=2147483651)
│  ├──297.13 MB (68.43%) -- active/window(http://localhost:8001/)
│  │  ├──296.85 MB (68.36%) -- js-compartment(http://localhost:8001/)
│  │  │  ├──296.46 MB (68.27%) -- classes
│  │  │  │  ├──260.36 MB (59.96%) -- class(ArrayBuffer)/objects
│  │  │  │  │  ├──256.00 MB (58.95%) ── non-heap/elements/asm.js
│  │  │  │  │  ├────4.36 MB (01.00%) ── malloc-heap/elements/normal
│  │  │  │  │  └────0.00 MB (00.00%) ── gc-heap
│  │  │  │  ├───34.94 MB (08.05%) -- class(WebAssembly.Instance)/objects
│  │  │  │  │   ├──23.02 MB (05.30%) ── non-heap/code/asm.js
│  │  │  │  │   ├──11.93 MB (02.75%) -- malloc-heap
│  │  │  │  │   │  ├──11.93 MB (02.75%) ── misc
│  │  │  │  │   │  └───0.00 MB (00.00%) ── slots
│  │  │  │  │   └───0.00 MB (00.00%) ── gc-heap
│  │  │  │  └────1.15 MB (00.27%) ++ (7 tiny)
│  │  │  └────0.40 MB (00.09%) ++ (6 tiny)
│  │  └────0.27 MB (00.06%) ++ (4 tiny)
│  └────0.37 MB (00.09%) ++ js-zone(0x11fd98800)
├───62.30 MB (14.35%) -- js-non-window
│   ├──50.80 MB (11.70%) -- runtime
│   │  ├──46.78 MB (10.77%) ── shared-immutable-strings-cache
│   │  └───4.03 MB (00.93%) ++ (11 tiny)
│   ├──10.20 MB (02.35%) -- zones
│   │  ├───8.86 MB (02.04%) ++ zone(0x10ed32800)
│   │  └───1.35 MB (00.31%) ++ (2 tiny)
│   └───1.29 MB (00.30%) ++ gc-heap
├───33.34 MB (07.68%) ── heap-unclassified
├───32.04 MB (07.38%) -- dom
│   ├──32.02 MB (07.37%) -- memory-file-data/large
│   │  ├──32.00 MB (07.37%) ── file(length=24416174, sha1=909afc5b6ac6e14aba8d4a8eddd7be8f9333167c)
│   │  └───0.02 MB (00.00%) ── file(length=14274, sha1=d3fdd457956c4c142006a6325137929288322aef)
│   └───0.03 MB (00.01%) ++ (2 tiny)
├────5.19 MB (01.19%) ++ heap-overhead
└────3.86 MB (00.89%) ++ (17 tiny)
(Reporter)

Comment 1

2 years ago
Created attachment 8798045 [details]
Test case

Updated

2 years ago
Blocks: 1282407

Updated

2 years ago
Flags: needinfo?(amarchesini)

Comment 2

2 years ago
I assume you don't have any smaller testcase?

Comment 3

2 years ago
[Tracking Requested - why for this release]:
Bug 1282407 is in FF50.
status-firefox50: --- → ?
status-firefox51: --- → ?
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Tracking 52+ for this memory related issue.
tracking-firefox52: ? → +

Comment 5

2 years ago
So something somewhere is keeping nsHostObjectURI object alive.
I guess revoking the url should eventually clear mBlobImpl.
Deterministic behavior would be nice, but not sure we can have that here, but just need to rely on nsExpirationTracker or some timer.
This code does this:

var e = new Blob([...something here...]);
var n = document.createElement("script");
n.src = URL.createObjectURL(e);
document.body.appendChild(n);
n.onload = function() { URL.revokeObjectURL(n.src); };

because the nsIURI of the BlobURL is also a nsIURIWithBlobImpl, and because we store the src URI in nsIScriptElement::mUri, we keeps the blob alive.

A way to fix this is to change nsIScriptElement::GetScriptURI() in nsIScriptElement::GetScriptURIAndForget() where we nullify mUri after being used.
Flags: needinfo?(amarchesini) → needinfo?(bugs)

Comment 7

2 years ago
Well, I assume we have many other places too where we have strong reference to nsIURI.
Flags: needinfo?(bugs)
Then, this is wontfix.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX

Comment 9

2 years ago
Whaat? no this isn't.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
The options I see are to backout bug 1282407 or add some kind of eviction mechanism so that revoked nsHostObjectURI objects eventually drop the pointer to the blob.
(Reporter)

Comment 11

2 years ago
(In reply to Olli Pettay [:smaug] from comment #2)
> I assume you don't have any smaller testcase?

Sorry, I realize I definitely should have walked through the test case since it is not that small. The page is a web export of an empty Unity project, which comes down to 24MB asm.js at the moment (wasm can't come too soon..), which is in the file test_nostrip.js. This script file is loaded in via XHR from JS code, and URL.createObjectURL() is used to create a blob of the code, which is added as a script element from the blob source. After the loading succeeds, it is released via URL.revokeObjectURL().

This process occurs in the file UnityLoader.js, at the very top in function LoadJSCodeBlob(e, t). The actual contents of the big 24MB test_nostrip.js file doesn't matter much for the purposes of this bug, it's practically to provide "meat" to show in about:memory, so it matches a real world case.

Being able to reclaim the memory usage via URL.revokeObjectURL() from the blob URL is critical for asm.js games purposes, because these script blobs can take on the order of 100MB of memory, and game assets can use a similar architecture, and their sizes can be larger than that. This makes the effect of the memory pinned in use here strikingly noticeable in practice, and large memory pressure contributes towards OOM issues we are actively fighting to optimize.

I tried to understand the original bug conversation that introduced the regression, but I'm not sure if I follow all the aspects there. It would be most preferable if URL.revokeObjectURL() immediately&eagerly freed the used memory, instead of e.g. waiting for a lazy GC (or similar?), because this would help us the most to reduce address space fragmentation induced OOM causes. Does that sound like a feasible way?
(In reply to Jukka Jylänki from comment #11)
> Being able to reclaim the memory usage via URL.revokeObjectURL() from the
> blob URL is critical
yes, of course.


> I tried to understand the original bug conversation that introduced the
> regression, but I'm not sure if I follow all the aspects there. It would be
> most preferable if URL.revokeObjectURL() immediately&eagerly freed the used
> memory, instead of e.g. waiting for a lazy GC (or similar?), because this
> would help us the most to reduce address space fragmentation induced OOM
> causes. Does that sound like a feasible way?
No. That is what caused bug 1282407. We need to keep the blob alive for some time, since the bloburl can be still used elsewhere, but probably not too long.
But for FF50, the options are to backout bug 1282407 or fix this using some timer or similar approach.
(Reporter)

Comment 13

2 years ago
(In reply to Olli Pettay [:smaug] from comment #12)
> No. That is what caused bug 1282407. We need to keep the blob alive for some
> time, since the bloburl can be still used elsewhere, but probably not too
> long.

Right, that makes sense. I wonder if it would be possible to tie this kind of timer mechanism into out-of-memory events, so that, say, if JS code attempts to allocate a very large ArrayBuffer and that fails, that any blobs waiting for a timer would then be immediately reclaimed and the ArrayBuffer allocation reattempted before giving up on the allocation?

I'm concerned that if the effects of URL.revokeObjectURL() were delayed on a timer, that it might cause memory pressure and risk OOMing subsequent large allocations that might happen shortly afterwards on the page.
Created attachment 8798466 [details] [diff] [review]
blob_revoke.patch
Assignee: nobody → amarchesini
Attachment #8798466 - Flags: review?(bugs)
(In reply to Jukka Jylänki from comment #13)
> Right, that makes sense. I wonder if it would be possible to tie this kind
> of timer mechanism into out-of-memory events, 
Unfortunately we don't really have good out-of-memory events
Comment on attachment 8798466 [details] [diff] [review]
blob_revoke.patch

yeah, don't have really better suggestions.

We need this on aurora and beta, right?
Attachment #8798466 - Flags: review?(bugs) → review+

Comment 17

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a43428c193
nsHostObjectURI must release BlobImpl when the underlying blob URL is revoked, r=smaug
Jukka, could you test the Nightly with the patch, I guess tomorrow or later, kiitos.

And we need this on branches too.

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a7a43428c193
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Reporter)

Comment 20

2 years ago
Neat, splendid work!

In current Nightly, I'm now getting this graph on the test case:

362.18 MB (100.0%) -- explicit
├──295.19 MB (81.50%) -- window-objects
│  ├──294.89 MB (81.42%) -- top(file:///C:/Users/clb/Downloads/codeblob_revoke.tar/codeblob_revoke/index.html, id=4294967301)
│  │  ├──293.96 MB (81.16%) -- active/window(file:///C:/Users/clb/Downloads/codeblob_revoke.tar/codeblob_revoke/index.html)
│  │  │  ├──293.67 MB (81.08%) -- js-compartment(file:///C:/Users/clb/Downloads/codeblob_revoke.tar/codeblob_revoke/index.html)
│  │  │  │  ├──293.50 MB (81.04%) -- classes
│  │  │  │  │  ├──260.36 MB (71.89%) -- class(ArrayBuffer)/objects
│  │  │  │  │  │  ├──256.00 MB (70.68%) ── non-heap/elements/wasm
│  │  │  │  │  │  ├────4.36 MB (01.20%) ── malloc-heap/elements/normal
│  │  │  │  │  │  └────0.00 MB (00.00%) ── gc-heap
│  │  │  │  │  ├───32.50 MB (08.97%) -- class(WebAssembly.Instance)/objects
│  │  │  │  │  │   ├──23.87 MB (06.59%) ── non-heap/code/wasm
│  │  │  │  │  │   ├───8.63 MB (02.38%) -- malloc-heap
│  │  │  │  │  │   │   ├──8.63 MB (02.38%) ── misc
│  │  │  │  │  │   │   └──0.00 MB (00.00%) ── slots
│  │  │  │  │  │   └───0.00 MB (00.00%) ── gc-heap
│  │  │  │  │  └────0.64 MB (00.18%) ++ (6 tiny)
│  │  │  │  └────0.17 MB (00.05%) ++ (6 tiny)
│  │  │  └────0.29 MB (00.08%) ++ (4 tiny)
│  │  └────0.93 MB (00.26%) ++ js-zone(0x200a8a12000)
│  └────0.30 MB (00.08%) ++ top(about:blank, id=4294967297)
├───31.90 MB (08.81%) -- js-non-window
│   ├──13.82 MB (03.81%) -- runtime
│   │  ├───9.46 MB (02.61%) ── shared-immutable-strings-cache
│   │  └───4.36 MB (01.20%) ++ (11 tiny)
│   ├───9.28 MB (02.56%) -- zones
│   │   ├──7.71 MB (02.13%) ++ zone(0x2009c828000)
│   │   └──1.57 MB (00.43%) ++ (4 tiny)
│   └───8.80 MB (02.43%) -- gc-heap
│       ├──5.00 MB (01.38%) ── unused-chunks
│       └──3.80 MB (01.05%) ++ (2 tiny)
├───24.97 MB (06.89%) ── heap-unclassified
├────6.05 MB (01.67%) -- heap-overhead
│    ├──4.71 MB (01.30%) ── bin-unused
│    └──1.34 MB (00.37%) ++ (2 tiny)
└────4.06 MB (01.12%) ++ (18 tiny)

This is actually our best memory usage so far on empty (256MB-heap) Unity project, so pretty darn good! The 24.97MB heap-unclassified is partly due to ANGLE, and discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1309309. The shared-immutable-strings-cache is also expected to slim down a bit with wasm, and also perhaps the class(WebAssembly.Instance)/objects section as well.
baku, we need the patch on branches, right?
Flags: needinfo?(amarchesini)
Comment on attachment 8798466 [details] [diff] [review]
blob_revoke.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1282407
[User impact if declined]: we can keep in memory blobs also when their blob URLs have been revoked. This is not a real leak: nsIURI could keep a reference to the blob if this URI is also a nsIURIWithBlobImpl. But in our code base, there are objects that keep alive URIs for several reasons. In this case, a simple <script url="a blob URL" /> can reproduce this leak.
[Describe test coverage new/current, TreeHerder]: Not easy to detect with a mochitest.
[Risks and why]: none: when a BlobURL is revoked, each nsIURIWithBlobImpl, pointing to that BlobURL, releases the BlobImpl object after 1 second. 
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8798466 - Flags: approval-mozilla-beta?
Attachment #8798466 - Flags: approval-mozilla-aurora?
Track 51+ as regression.
status-firefox50: ? → affected
status-firefox51: ? → affected
tracking-firefox51: ? → +
Regression in 50, tracking for that release.
tracking-firefox50: ? → +
Comment on attachment 8798466 [details] [diff] [review]
blob_revoke.patch

Fixes a mem leak, has been in Nightly for a week, Aurora51+, Beta50+
Attachment #8798466 - Flags: approval-mozilla-beta?
Attachment #8798466 - Flags: approval-mozilla-beta+
Attachment #8798466 - Flags: approval-mozilla-aurora?
Attachment #8798466 - Flags: approval-mozilla-aurora+

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3eefefba72cc
status-firefox51: affected → fixed

Comment 27

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/015d719318b9
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.