1464648 - Bail early when parsing empty form data (to avoid NS_NOTREACHED assertion failure)

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

I'm slowly replacing uses of XPCOM's non-fatal NS_NOTREACHED with MFBT's fatal MOZ_ASSERT_UNREACHABLE. This NS_NOTREACHED("Should never reach here.") assertion:

https://searchfox.org/mozilla-central/rev/14578d6f2d2ec5246572827061ecefa60a40855d/dom/base/BodyUtil.cpp#390

fails on the following test case of the testing/web-platform/tests/fetch/api/response/response-consume-empty.html web platform test:

checkResponseWithNoBody("formData with correct multipart type (error case)", checkBodyFormDataError, [["Content-Type", 'multipart/form-data; boundary="boundary"']]);

https://searchfox.org/mozilla-central/rev/b47af5169898b156429c8cdaf415afdaa3c43a7b/testing/web-platform/tests/fetch/api/response/response-consume-empty.html#82-83

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1464648 - Bail early when parsing empty form data (to avoid NS_NOTREACHED assertion failure).

This NS_NOTREACHED("Should never reach here.") assertion fails on the following test case of the testing/web-platform/tests/fetch/api/response/response-consume-empty.html web platform test:

checkResponseWithNoBody("formData with correct multipart type (error case)", checkBodyFormDataError, [["Content-Type", 'multipart/form-data; boundary="boundary"']]);

Review commit: https://reviewboard.mozilla.org/r/247100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247100/

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8980951 [details]
Bug 1464648 - Bail early when parsing empty form data (to avoid NS_NOTREACHED assertion failure).

https://reviewboard.mozilla.org/r/247100/#review253432

::: dom/base/BodyUtil.cpp:332
(Diff revision 1)
>      mData.BeginReading(start);
>      // This should ALWAYS point to the end of data.
>      // Helpers make copies.
>      mData.EndReading(end);
>  
> -    while (start != end) {
> +    if (start == end) {

What about this check at the beginning of Parse() ?

if (mData.IsEmpty()) {
  return false;
}

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Andrea Marchesini [:baku] from comment #2)
> What about this check at the beginning of Parse() ?
> 
> if (mData.IsEmpty()) {
>   return false;
> }

Good point! We don't need to parse mMimeType if there is no form data to parse. :)

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 8980951 [details]
Bug 1464648 - Bail early when parsing empty form data (to avoid NS_NOTREACHED assertion failure).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247100/diff/1-2/

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8980951 [details]
Bug 1464648 - Bail early when parsing empty form data (to avoid NS_NOTREACHED assertion failure).

https://reviewboard.mozilla.org/r/247100/#review254614

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/578f9a189eb8
Bail early when parsing empty form data (to avoid NS_NOTREACHED assertion failure). r=baku

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/578f9a189eb8