1179396 - Assert/Crash running "fetch-csp.https.html" wpt service worker test in Nightly

Status: RESOLVED DUPLICATE of bug 1184557

(Core :: JavaScript Engine, defect)

Description

[Noemí Freire (:noemi)]

Attachment: Nightly_Crash_wpt_sw_fetch-csp.https.rtf

A Nightly crash occurs when executing "fetch-csp.https.html" wpt test such as |./mach web-platform-tests _mozilla/service-workers/service-worker/fetch-csp.https.html| with today's (7/1) master build

The assertion failure shown is as follows:
"Assertion failure: false (unknown ownership), at /Users/noef/Documents/mozilla-central/js/src/vm/StructuredClone.cpp:430"

Please find attached the crash report corresponding to this

Comment 1

[(no longer active)]

This is hitting the "unknown ownership" assertion in DiscardTransferables.  Steve, can you please take a look?

To reproduce, run the following command in a debug build:

$ ./mach web-platform-tests testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-csp.https.html

Thanks!

Comment 2

[Noemí Freire (:noemi)]

Hi,

Just adding that the same assertion/crash occurs while executing "fetch-request-xhr.https.html" test.

Comment 3

[Noemí Freire (:noemi)]

(In reply to Noemí Freire (:noemi) from comment #2)
> Hi,
> 
> Just adding that the same assertion/crash occurs while executing
> "fetch-request-xhr.https.html" test.

and also when running "fetch-response-xhr.https.html" test

Comment 4

[Noemí Freire (:noemi)]

(In reply to Noemí Freire (:noemi) from comment #3)
> (In reply to Noemí Freire (:noemi) from comment #2)
> > Hi,
> > 
> > Just adding that the same assertion/crash occurs while executing
> > "fetch-request-xhr.https.html" test.
> 
> and also when running "fetch-response-xhr.https.html" test

and also when running:
* "invalid-blobtype.https.html" test
* "referer.https.html" test

Comment 5

[Steve Fink [:sfink] [:s:]]

:baku is fixing a very similar problem in WorkerPrivate.cpp. I'm not sure whether he'll be adding the JSAutoStructuredCloneBuffer.setCallbacks() call we were discussing, or if he'll fix it by using the constructor instead (which is currently the only way to set the callbacks.) But whatever he does there will probably be relevant here too.

What is happening is that you have a JSAutoStructuredCloneBuffer that was created without having any nondefault data type handlers set. If you read the data out of it, then you'll be fine because the read call takes in a callback pointer. But if the buffer just dies (due to an error or something, so the read is not invoked), then the JSAutoStructuredCloneBuffer destructor will try to free any Transferables in the buffer, but it won't have the callbacks it needs to recognize what's in there. So it'll throw the above assertion.

I know this API is awful. I have a long-abandoned patch in bug 991857 to clean up the handler registration API. But in the meantime, there are two options: (1) arrange for your buffer to be constructed with the needed callbacks, or (2) add a setCallbacks method so that you can fix up a clone buffer after the fact. Both may require fixing JSAutoStructuredCloneBuffer's copy constructors to copy over the callbacks, I'm not sure. Or rather, in your case I'm pretty sure that doesn't matter, but in baku's case it might.

In your case, the clone buffer is the mBuffer field of PostMessageEvent. It is constructed with the default constructor. If you constructed it with &sPostMessageCallbacks then this would all be happier. Except that I'm not sure how the 'closure' arg is handled. It looks like you need it for freeing these guys (it force-closes a port), but it needs to contain the event, and I don't follow the code well enough to understand when that is or is not available. This is feeling a little shaky -- PostMessageEvent contains a JSAutoStructuredCloneBuffer, which means that destroying the PME will destroy the buffer, but the buffer's destruction requires a pointer to the event. Perhaps the event should be stored in the buffer itself, via the 64 bits of "extraData" available? It's unpleasant, I know, but if you need multiple pieces of information then you can heap-allocate a struct and manage its lifetime within the callbacks.

Comment 6

[(no longer active)]

baku, can you take this please?

Comment 7

[Andrea Marchesini [:baku]]

Yes. I'm working on a more generic solution. But, yes I can take this.

Comment 8

[Andrea Marchesini [:baku]]

The patch is ready in bug 1184557. I'm planning to replace all the custom JSStructuredCloneBuffer callback implementations with a more generic StructuredCloneHelper.

Comment 9

[Andrea Marchesini [:baku]]

I close this bug as duplicate of bug 1184557.

Comment 10

[Noemí Freire (:noemi)]

Hi,

just checked on m-c Nightly debug (d3228c82badd revision) and the result when running "fetch-csp.https.html" test is Timeout, please see the details below:

Harness status: Timeout
Found 1 test
1 Timeout
*Timeout: Verify CSP control of fetch() in a Service Worker

Adding this test to Bug 1180622. Thanks!

Comment 11

[Noemí Freire (:noemi)]

Hi,

just checked on m-c (fcef8ded8221 revision) and the test successfully runs. Thanks for fixing it!.

Summary

Harness status: OK

Found 1 tests
1 Pass
Details
Result	Test Name
Pass	Verify CSP control of fetch() in a Service Worker