ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=6023665130143744

Note: I couldn't reproduce a crash on 58.0.2989.0 canary on Mac OS X, but we'll see if Clusterfuzz can.

Git bisect showed that the crash is reproducible since turning on TraceWrappables in https://codereview.chromium.org/2592713002
I will try it on latest canary too.

The crash cannot be reproduced in 58 because https://codereview.chromium.org/2649513002 fixed it.
This commit is not included in version 57 though so I think this is a valid vulnerability for the current Dev channel (57).
It might be possible to reproduce it with another ThreadableLoaderClient, not XMLHttpRequest.
Probably the best would be to make ThreadableLoaderClient inherit from GarbageCollectedMixin and WorkerThreadableLoader should have a Member or WeakMember to it instead of a raw pointer.

Can you please CC yhirano@ or sof@ ? They were discussing something similar in the CL that fixed this a few days ago.

I have attached a new reproduction case which is probably more useful for ClusterFuzz.

The crash can be triggered with either:
1. run "./chrome xhrgc.html"; manually trigger the gc via devconsole
2. run "./chrome --js-flags=--expose-gc xhrgc.html"
3. run "./content_shell --run-layout-test xhrgc.html"

Deleted: xhrgc.html 410 bytes

xhrgc.html 410 bytes View Download

yhirano, could you please take a look at this? See Comment 5: sounds like it is fixed by your CL https://codereview.chromium.org/2649513002, but maybe that needs to be merged to 57.

#9: There are multiple implementations for ThreadableLoaderClient (not just XMLHttpRequest), I think this issue could be reproduced with the others as well.

Confirmed that https://codereview.chromium.org/2649513002 addresses, yhirano@ will no doubt request a M57 merge at first available opportunity.

A real fine bug report & test, btw - we should add it. Issue 667254 (which might not be visible) was about locating the loader client that didn't cleanly detach from its loader. XHR was the one culprit identified.

The problem happens because in some cases (I don't know exactly what they are though), ContextLifecycleObserver::contextDestroyed is not called. Some ThreadableLoaderClient expects it to be called (in combination with ActiveScriptWrappable::hasPendingActivity). I believe that the expectation should be valid and there is an infra-issue. +haraken@.

This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

[Bulk edit]

A friendly reminder that M57 Beta launch is coming soon on February 2nd (in a week)! Your bug is labelled as Beta ReleaseBlock, pls make sure to land the fix and get it merged into the release branch (2987) ASAP so it gets enough baking time in Dev (before Beta promotion). Thank you!

See https://bugs.chromium.org/p/chromium/issues/detail?id=667254#c48 for the M57 merge.

There is another bug regarding this same issue.
I do not want to file another report until this is visible to the public.

Sometimes if an XMLHttpRequest is started from a worker, it is prematurely GC'd before it can finish.
If you run "./content_shell --run-layout-test xhrgc2.html" with my now attached file then it should report that there is a text field "error", but it does not because GC destroys blink::XMLHttpRequest and it has a prefinalizer that cancels the loader (before yhirano's patch it did not have a prefinalizer but the WorkerThreadable loader was GC'd as well, and when it did not crash it stopped the downlaod).

This is a regression in 57. Before 57 the TraceWrappables runtime feature was disabled (https://crrev.com/2592713002) and somehow that makes it work. I checked what happens if I remove the "status=stable" part from the TraceWrappables line in //third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in and it works correctly again. (running the layout test reports the text field "error")
I'm not familiar with Blink internals, I do not know what's supposed to keep the XMLHttpRequest alive, but something really should.

This bug kind of makes XMLHttpRequests unusable from workers so I think it will affect lots of projects.
(For example https://web.tresorit.com can not download files at all because of this bug).

Please fix this issue too or disable TraceWrappables as a temporary workaround.

Deleted: xhrgc2.html 513 bytes

xhrgc2.html 513 bytes View Download

Confirmed #19, but this is a separate (non-security) issue. The XHR object signals that it must be kept alive (hasPendingActivity() returns |true|), but the v8 GC fails to trace through its wrapper to mark and keep the XHR object alive.

Handled as-wanted if the test snippet is instead run within the context of a document.

If it's prematurely collected it probably has to do with pending activities. I think wrapper tracing by now uses a different notion of active scruptwrappable (see MajorGCWrapperVisitor before I deleted it). 

I am a bit low on cycles this week (traveling + talk) but will have a look early next week if the issue still persists.

Thanks sigbjorn for investigating! That's what I thought. 

(Written at 39k feet altitude :))

Removing ReleaseBlock-Beta per #17

#23: as far as i could gather, the weak that DOMWrapperMap::set() created in a worker setting, wasn't being retained when the associated ScriptWrappable::hasPendingActivity() returned |true|. Which precipitated it not being traced through later on.

Yeah, this is related to wrapper worlds.

As far as I can see the bug is an artifact from the prototype. We marked only the main world wrapper, where we actually should have put it onto the marking deque, effectively triggering processing in all worlds.

I am about to verify that theory.

Alright, the problem seems to be more subtle.

Workers are not running on the main thread and are not being stored in the main world wrapper. Since we currently check for main thread when marking wrappers in all worlds, we will never mark through the DOMWrapperMap in workers.

Thinking of a proper fix now.

FWIW, workers don't have DOM trees, so we won't need to support incremental tracing.

#29: True, but we can just reuse the existing infra.

The problem is that the worker world is not stored anywhere. So we only use the main world wrapper only for the main world and not workers. Worker worlds are also not part of the isolated world map, so we cannot reach it from there.

We could (a) make WorkerWorld part of the isolatedWorldsMap and trace (b) all isolated worlds in markWrappersInAllWorlds. Locally, this fixes the issue.

haraken/jochen: Any reason we wouldn't be allowed to do (a) ?

Can we create a separate map for worker worlds (e.g., workerWorldsMap)?

Conceptually:

- The main thread has its main world (WindowProxy::m_world) and isolated worlds.
- A worker thread has its own world (WorkerOrWorkletScriptController::m_world).

In that sense, it looks a bit strange to classify worker's world as isolated worlds.

Sorry I'm confused.

Why do we need to visit wrappers in the worker world when we run a GC on the main thread? V8 heaps are completely separated per thread, so we won't need to worry about wrappers in a worker (or the main thread) when running a GC on the main thread (or a worker).

as far as I understand the bug here is that we're not visiting those wrappers when doing a GC in the worker because of the incorrect assumption that wrappres in the worker thread are stored in ScriptWrappables

Ah, makes sense. Then can we visit WorkerOrWorkletScriptController::m_world when we run a GC on a worker thread?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp?rcl=1485491011&l=137 doesn't have access to that, but that should be easy to fix

Really don't want to pre-empt mlippautz@'s work, but if it helps getting a change in sooner during the upcoming busy week, https://codereview.chromium.org/2663643002/ adds the worker special case.

 Issue 686563  handles the fix for the above.

The current CL (patchset 8) does solve the issue when I call "self.gc()" from the worker.
But the issue still persists if I click on the Trash icon in the DevConsole.

You can reproduce this by running "./chrome xhrgc3.html" with my now attached file and then manually triggering the GC from the DevConsole before the download finishes.

I have seen some cases where an automatically triggered GC kills the XHR but I cannot easily reproduce that.

Deleted: xhrgc3.html 395 bytes

xhrgc3.html 395 bytes View Download

Thanks for all your efforts. Will take a look today.

The crasher and security issue here has been fixed. 

Lets move the discussion about correctness and the corresponding fix to  issue 686563 . What is left is properly tracing through wrappers in workers which is tracked by the other issue.

Does this issue qualify for the reward program?
If so, please add the label reward-topanel.
Or is it only a duplicate of bug 667254?
(I have no permission to see that bug, so I cannot tell for sure.)
Thanks!

Given the repro steps and the useful test cases, I am adding the label for now. If somebody disagrees, please take it off.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please merge your change to M57 branch 2987 before 5:00 PM PT, Monday (02/06/) so we can pick it up for next Beta release. Thank you.

govind@ Both fixes are already merged into 2987
This is the fix for the UAF: https://crrev.com/995de01d
This is the fix for the non-security part: https://crrev.com/730c14f1

Per comment #49, both fixes are merged to M57.

Congratulations! The panel decided to award $3,000 in this case.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot