943444 - Fatal CC asserts when using responseType="json" with worker XHR

Status: RESOLVED WORKSFORME

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Security-sensitive for now.

STEPS TO REPRODUCE:

1)  Load attached testcase in a debug build.
2)  Hit reload button.
3)  If by some chance you don't crash, hit reload button again.

All the testcase does is run this in a worker:

var xhr = new XMLHttpRequest;
xhr.open("GET", "data:text/plain,{}", false);
xhr.responseType = "json";
xhr.send();

what I get is:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
[Switching to process 28070 thread 0xb917]
AssertNoGcThing (aGCThing=0x14c7400a0, aName=0x107bbe80d "mStateData.mResponse", aClosure=0x0) at CycleCollectedJSRuntime.cpp:833
833       MOZ_ASSERT(!aGCThing);
(gdb) bt
#0  AssertNoGcThing (aGCThing=0x14c7400a0, aName=0x107bbe80d "mStateData.mResponse", aClosure=0x0) at CycleCollectedJSRuntime.cpp:833
#1  0x0000000103b34bbe in TraceCallbackFunc::Trace (this=0x14b1f8958, p=0x115e5db50, name=0x107bbe80d "mStateData.mResponse", closure=0x0) at nsCycleCollectionParticipant.cpp:72
#2  0x00000001058bd4ba in mozilla::dom::workers::XMLHttpRequest::cycleCollection::Trace (this=0x1094e1c90, p=0x115e5daa0, aCallbacks=@0x14b1f8958, aClosure=0x0) at XMLHttpRequest.cpp:1483
#3  0x0000000103b7422b in mozilla::CycleCollectedJSRuntime::AssertNoObjectsToTrace (this=0x14b200b28, aPossibleJSHolder=0x115e5daa0) at CycleCollectedJSRuntime.cpp:841
#4  0x0000000103b7a154 in nsCycleCollector::CollectWhite (this=0x149742000) at nsCycleCollector.cpp:2444

In particular, mStateData.mResponse is an object value in this case.

Comment 1

[Boris Zbarsky [:bzbarsky]]

It looks like mStateData.mResponse is traced but not unlinked.  Is that what the assert is about?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Yes, most probably.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Do we require unlinking all traced things?  That's non-obvious to me, and probably others...

Is this a security issue, or can we just open it up?

Comment 4

[Andrew McCreight [:mccr8]]

If a traced pointer isn't cleared in Unlink, and we have a bug somewhere else that makes an object reachable from content after it is unlinked (we've had a ton of these), then you have a use-after-free bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 6

[Andrew McCreight [:mccr8]]

Well, potentially.  That's assuming that we stop actually tracing the object after the unlink.  If you do a DROP in the destructor, then you should still be okay, and at most you just have an unlink.

Comment 7

[Andrew McCreight [:mccr8]]

> and at most you just have an unlink.
have a leak, not an unlink...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Does this mean we have no responseType="json" coverage on workers, btw?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Does this mean we have no responseType="json" coverage on workers, btw?

Probably.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Happy to review a patch here if somebody knows what needs to be done.

Comment 11

[Andrew McCreight [:mccr8]]

I think this isn't actually a security problem.  The DropJSObjects call is only in the destructor, so we'll continue tracing this object pointer even after the object is unlinked. I'll unhide it in a few days if nobody objects.

Comment 12

[Andrew McCreight [:mccr8]]

I wrote a patch that does the simple thing of setting mStateData.mResponse to undefined in Unlink, but the test I added, which is just bz's test case, fails with this about 80% of the time:

Assertion failure: mRooted (Mismatched calls to Unpin!), at ../../../dom/workers/XMLHttpRequest.cpp:1757
TEST-UNEXPECTED-FAIL | /tests/dom/workers/test/test_bug943444.html | application terminated with exit code 11
PROCESS-CRASH | /tests/dom/workers/test/test_bug943444.html | application crashed [@ mozilla::dom::workers::XMLHttpRequest::Unpin()]
Return code: 1

I'll try to figure out what is supposed to be happening here.  My theory would be that something is keying off the value of request somehow, but I don't really understand the behavior of all these interlocking unpinning runnables.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Is this still an issue?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Does not reproduce with the attached test case.