1341678 - Assert in dom/test/InternalRequest.h running web-platform-tests

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[James Graham [:jgraham]]

https://treeherder.mozilla.org/logviewer.html#?job_id=79384302&repo=try&lineNumber=4134

[task 2017-02-22T15:34:33.330556Z] 15:34:33     INFO - TEST-START | /fetch/api/request/request-disturbed.html
[task 2017-02-22T15:34:33.461989Z] 15:34:33     INFO - PROCESS | 2562 | ++DOCSHELL 0x7fd5ea926800 == 7 [pid = 2625] [id = {7c173f9d-ddac-4810-b56f-c083d875766b}]
[task 2017-02-22T15:34:33.462097Z] 15:34:33     INFO - PROCESS | 2562 | ++DOMWINDOW == 20 (0x7fd5eb585400) [pid = 2625] [serial = 45] [outer = (nil)]
[task 2017-02-22T15:34:33.537724Z] 15:34:33     INFO - PROCESS | 2562 | ++DOMWINDOW == 21 (0x7fd5f998f000) [pid = 2625] [serial = 46] [outer = 0x7fd5eb585400]
[task 2017-02-22T15:34:33.639433Z] 15:34:33     INFO - PROCESS | 2562 | ++DOMWINDOW == 22 (0x7fd5fbe22800) [pid = 2625] [serial = 47] [outer = 0x7fd5eb585400]
[task 2017-02-22T15:34:33.922322Z] 15:34:33     INFO - PROCESS | 2562 | Assertion failure: !mBodyStream, at /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/InternalRequest.h:468
[task 2017-02-22T15:34:34.030640Z] 15:34:34     INFO - PROCESS | 2562 | #01: mozilla::dom::RequestBinding::_constructor(JSContext*, unsigned int, JS::Value*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.038964Z] 15:34:34     INFO - PROCESS | 2562 | #02: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.047177Z] 15:34:34     INFO - PROCESS | 2562 | #03: js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.053690Z] 15:34:34     INFO - PROCESS | 2562 | #04: InternalConstruct(JSContext*, js::AnyConstructArgs const&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.060225Z] 15:34:34     INFO - PROCESS | 2562 | #05: Interpret(JSContext*, js::RunState&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.067105Z] 15:34:34     INFO - PROCESS | 2562 | #06: js::RunScript(JSContext*, js::RunState&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.073377Z] 15:34:34     INFO - PROCESS | 2562 | #07: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.092924Z] 15:34:34     INFO - PROCESS | 2562 | #08: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.100910Z] 15:34:34     INFO - PROCESS | 2562 | #09: js::fun_apply(JSContext*, unsigned int, JS::Value*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.101998Z] 15:34:34     INFO - PROCESS | 2562 | #10: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.108328Z] 15:34:34     INFO - PROCESS | 2562 | #11: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.114918Z] 15:34:34     INFO - PROCESS | 2562 | #12: Interpret(JSContext*, js::RunState&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.115975Z] 15:34:34     INFO - PROCESS | 2562 | #13: js::RunScript(JSContext*, js::RunState&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.116702Z] 15:34:34     INFO - PROCESS | 2562 | #14: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.117615Z] 15:34:34     INFO - PROCESS | 2562 | #15: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.122896Z] 15:34:34     INFO - PROCESS | 2562 | #16: PromiseReactionJob(JSContext*, unsigned int, JS::Value*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.123936Z] 15:34:34     INFO - PROCESS | 2562 | #17: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.124823Z] 15:34:34     INFO - PROCESS | 2562 | #18: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.125623Z] 15:34:34     INFO - PROCESS | 2562 | #19: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.132443Z] 15:34:34     INFO - PROCESS | 2562 | #20: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.142650Z] 15:34:34     INFO - PROCESS | 2562 | #21: mozilla::dom::PromiseJobCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.144202Z] 15:34:34     INFO - PROCESS | 2562 | #22: mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.149306Z] 15:34:34     INFO - PROCESS | 2562 | #23: PromiseJobRunnable::Run() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.154820Z] 15:34:34     INFO - PROCESS | 2562 | #24: mozilla::dom::Promise::PerformMicroTaskCheckpoint() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.159053Z] 15:34:34     INFO - PROCESS | 2562 | #25: mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.165759Z] 15:34:34     INFO - PROCESS | 2562 | #26: XPCJSContext::AfterProcessTask(unsigned int) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.171664Z] 15:34:34     INFO - PROCESS | 2562 | #27: nsThread::ProcessNextEvent(bool, bool*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.177096Z] 15:34:34     INFO - PROCESS | 2562 | #28: NS_ProcessNextEvent(nsIThread*, bool) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.183230Z] 15:34:34     INFO - PROCESS | 2562 | #29: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.188728Z] 15:34:34     INFO - PROCESS | 2562 | #30: MessageLoop::RunInternal() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.194391Z] 15:34:34     INFO - PROCESS | 2562 | #31: MessageLoop::Run() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.200477Z] 15:34:34     INFO - PROCESS | 2562 | #32: nsBaseAppShell::Run() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.206190Z] 15:34:34     INFO - PROCESS | 2562 | #33: XRE_RunAppShell() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.212448Z] 15:34:34     INFO - PROCESS | 2562 | #34: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.213655Z] 15:34:34     INFO - PROCESS | 2562 | #35: MessageLoop::RunInternal() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.214582Z] 15:34:34     INFO - PROCESS | 2562 | #36: MessageLoop::Run() (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.221526Z] 15:34:34     INFO - PROCESS | 2562 | #37: XRE_InitChildProcess(int, char**, XREChildData const*) (/home/worker/workspace/build/application/firefox/libxul.so)
[task 2017-02-22T15:34:34.273275Z] 15:34:34     INFO - PROCESS | 2562 | #38: content_process_main(mozilla::Bootstrap*, int, char**) (/home/worker/workspace/build/application/firefox/firefox)
[task 2017-02-22T15:34:34.273411Z] 15:34:34     INFO - PROCESS | 2562 | #39: main (/home/worker/workspace/build/application/firefox/firefox)
[task 2017-02-22T15:34:36.685282Z] 15:34:36     INFO - PROCESS | 2562 | #40: __libc_start_main (/build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:325)
[task 2017-02-22T15:34:36.685389Z] 15:34:36     INFO - PROCESS | 2562 | #41: _start (/home/worker/workspace/build/application/firefox/firefox)
[task 2017-02-22T15:34:36.685484Z] 15:34:36     INFO - PROCESS | 2562 | [Parent 2562] WARNING: pipe error (84): Connection reset by peer: file /home/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 346
[task 2017-02-22T15:34:36.685589Z] 15:34:36     INFO - PROCESS | 2562 | [Parent 2562] WARNING: pipe error (78): Connection reset by peer: file /home/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 346
[task 2017-02-22T15:34:36.685645Z] 15:34:36     INFO - PROCESS | 2562 | 
[task 2017-02-22T15:34:36.690109Z] 15:34:36     INFO - PROCESS | 2562 | ###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I believe this is probably triggering on this line:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/request/request-disturbed.html#70

The assertion does not take into account that we can already have a body from a copied request, but then override it in the initializer as well.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make Request::Constructor() expect over-writing a copied body with an initializer. r=asuth

Explicitly handle the condition of over-writing the copied body.  This seems safer than just dropping the assertion.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update wpt test expectations now that we don't assert. r=asuth

Update the WPT test expectations.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5e3b83ade732b448b42868481fd0a27d8b910c

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8840005 [details] [diff] [review]
P1 Make Request::Constructor() expect over-writing a copied body with an initializer. r=asuth

Review of attachment 8840005 [details] [diff] [review]:
-----------------------------------------------------------------

Restating for my benefit:
- Request::Constructor can have aInput=(an existing request with an undisturbed body) and aInit=(a dictionary including a body).
- In this aInput=(existing request) case, the conditional extracts the InternalRequest, and the first statement afterwards is GetRequestConstructorCopy which copies over many things including mBodyStream.
  - This body will be consumed and nulled at the bottom of the method regardless of whther aInit contains a body, corresponding to step 38.1 of the Request constructor (https://fetch.spec.whatwg.org/#dom-request), so we don't need to freak out about ownership issues.
- InternalRequest::SetBody has `MOZ_ASSERT_IF(aStream, !mBodyStream);` which is what this bug is addressing.
- temporaryBody was basically being used like a boolean up to the point it would get clobbered by `stream` anyways, the change to hasCopiedBody reduces confusion about what's going on.  (request's body is already aInput's body after the call to GetRequestConstructorCopy, so it never needs to be explicitly set.)
- In the aInit.mBody.WasPassed() case we now conditionally invoke request->SetBody(nullptr) in order to avoid the assertion when we do request->SetBody(temporaryBody);

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8840007 [details] [diff] [review]
P2 Update wpt test expectations now that we don't assert. r=asuth

Review of attachment 8840007 [details] [diff] [review]:
-----------------------------------------------------------------

Begone, test expectation smell!  (In the sense that was a huge, suspicious-looking hunk of conditions.  Is that something we have to do to disable things or just an artifact of automated test disablement from remotely extract JSON runs?)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc9257cda4d
P1 Make Request::Constructor() expect over-writing a copied body with an initializer. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ea3256dd8b
P2 Update wpt test expectations now that we don't assert. r=asuth

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out under suspicion of causing failures like https://treeherder.mozilla.org/logviewer.html#?job_id=80678512&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0b4db0c156

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I have a hard time seeing how this bug could cause those errors, but lets see what try says.

Before these patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e662d3df37474f2b37b7673027e774098db3f99f

After these patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bc84d582ae09b5aefcde36806f27123feaa4a3e

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I can't reproduce those failures in try build.  I'm going to reland.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82001a435bce
P1 Make Request::Constructor() expect over-writing a copied body with an initializer. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1adc95479dd1
P2 Update wpt test expectations now that we don't assert. r=asuth

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/82001a435bce
https://hg.mozilla.org/mozilla-central/rev/1adc95479dd1