Bug 94515 – [Soup] utilize multipart/x-mixed-replace support recently added to libsoup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gustavo Noronha (kov)]

[Soup] utilize multipart/x-mixed-replace support recently added to libsoup

[Gustavo Noronha (kov)]

Created attachment 159498 [details]
Patch

[Martin Robinson]

Comment on attachment 159498 [details]
Patch

Can we unskip some tests now?

[Gyuyoung Kim]

Comment on attachment 159498 [details]
Patch

Attachment 159498 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13543462

[Dan Winship]

Comment on attachment 159498 [details]
Patch

>+LIBSOUP_REQUIRED_VERSION=2.39.6

you should go for 2.39.90 actually, which just now exists

[Gustavo Noronha (kov)]

(In reply to comment #2)
> (From update of attachment 159498 [details])
> Can we unskip some tests now?

Yes, the patch already unskips them =)

[Gustavo Noronha (kov)]

Created attachment 159529 [details]
Patch

[Martin Robinson]

Comment on attachment 159529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159529&action=review

Great stuff! I just see a few small nits. Holding off on the r+ until I understand the changes the skipped file

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:414
> +static void nextPartCallback(GObject* source, GAsyncResult* res, gpointer data)

Nit: res -> result

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:439
> +    if (!d->m_inputStream) {
> +            client->didFinishLoading(handle.get(), 0);
> +            g_input_stream_close_async(G_INPUT_STREAM(d->m_multipartInputStream.get()), G_PRIORITY_DEFAULT, 0, closeCallback, handle.get());
> +            return;
> +    }
> +

Another nit: the indentation looks a bit off here.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:442
> +    d->m_response.updateFromSoupMessageHeaders(const_cast<SoupMessageHeaders*>(soup_multipart_input_stream_get_headers(d->m_multipartInputStream.get())));

Instead of casting here, it makes sense to have updateFromSoupMessageHeaders accept a const.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:516
> +

Nit: extra line here.

> LayoutTests/platform/gtk/TestExpectations:1060
> +BUGWKGTK : http/tests/multipart/invalid-image-data.html = TEXT
> +BUGWKGTK : http/tests/multipart/load-last-non-html-frame.php = TEXT
> +BUGWKGTK : http/tests/multipart/multipart-replace-non-html-content.php = PASS
> +BUGWKGTK : http/tests/multipart/policy-ignore-crash.php = PASS
> +BUGWKGTK : http/tests/multipart/multipart-wait-before-boundary.html = PASS
> +BUGWKGTK : http/tests/multipart/stop-crash.html = PASS

Why can't we unskip these tests now?

[Jussi Kukkonen (jku)]

We agreed with kov that I could add EFL expectation changes to the patch (as efl 
uses libsoup as well) and rebase it. I did that but I ran into some problems:

First, bug 97565 crashes most of the EFL tests that cover this so I'll have to keep them in TestExpectations

Second, invalid-image-data.html and invalid-image-data-standalone.html still don't work (at least on EFL): there is no green image. Shouldn't this patch fix them?

I'll still upload my patch just in case it's useful for kov. 

Changes from kovs last patch:
* rebased
* updated jhbuilds to use soup 2.40 (stable release from three days ago)
* fixed nits from mrobinson (though see below)
* the const_cast still has to be done because of soup api but now
  it happens in updateFromSoupMessageHeaders
* modified efl TestExpectations/Skipped

I did not touch the gtk TestExpectations as I didn't understand what it meant, so those are still in the old format.

[Jussi Kukkonen (jku)]

Created attachment 165852 [details]
Patch

[Jussi Kukkonen (jku)]

Created attachment 165855 [details]
An even more up-to-date rebase

[WebKit Review Bot]

Attachment 165855 [details] did not pass style-queue:

WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
LayoutTests/platform/gtk/TestExpectations:1111:  use "#" instead of "//" for comments  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:1112:  unrecognized old-style bug identifier "BUGWKGTK"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:1113:  unrecognized old-style bug identifier "BUGWKGTK"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:1114:  unrecognized old-style bug identifier "BUGFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WKGTK"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:1115:  unrecognized old-style bug identifier "BUGWKGTK"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:1116:  unrecognized old-style bug identifier "BUGWKGTK"  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:1117:  unrecognized old-style bug identifier "BUGWKGTK"  [test/expectations] [5]
Total errors found: 7 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jussi Kukkonen (jku)]

(In reply to comment #8)
> Second, invalid-image-data.html and invalid-image-data-standalone.html still don't work (at least on EFL): there is no green image. Shouldn't this patch fix them?

Actually, invalid-image-data-standalone.html does work with the patch on EFL.

For some reason invalid-image-data.html does not: there is no green box, not even the outline is there.

[Jussi Kukkonen (jku)]

*** Bug 97664 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Comment on attachment 165855 [details]
An even more up-to-date rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=165855&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1020
> +            d->m_inputStream.leakRef();

Why do we need to leak this ref here?

[Jussi Kukkonen (jku)]

(In reply to comment #8)
> First, bug 97565 crashes most of the EFL tests that cover this so I'll have to keep them in TestExpectations

The patch in bug 97565 has allowed me to test this, and the tests do indeed avoid crashing and actually pass with the  patch from this bug and that one: The only problem in EFL after that is invalid-image-data.html. I'm continuing the investigation on that.

[Jussi Kukkonen (jku)]

Comment on attachment 165855 [details]
An even more up-to-date rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=165855&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-449
> -    d->m_inputStream = adoptGRef(in);

This seems to leak 'in' later on in some cases? There are a couple of possible function exit points before 'd->m_inputStream = adoptGRef(in)' is now called.

[Zan Dobersek]

*** Bug 30845 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Created attachment 168025 [details]
Patch

[Gustavo Noronha (kov)]

(In reply to comment #7)
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:414
> > +static void nextPartCallback(GObject* source, GAsyncResult* res, gpointer data)
> 
> Nit: res -> result

Oops, forgot this one.

> Another nit: the indentation looks a bit off here.

Fixed, the style checker is happy now =)

> > LayoutTests/platform/gtk/TestExpectations:1060
> > +BUGWKGTK : http/tests/multipart/invalid-image-data.html = TEXT
> > +BUGWKGTK : http/tests/multipart/load-last-non-html-frame.php = TEXT
> > +BUGWKGTK : http/tests/multipart/multipart-replace-non-html-content.php = PASS
> > +BUGWKGTK : http/tests/multipart/policy-ignore-crash.php = PASS
> > +BUGWKGTK : http/tests/multipart/multipart-wait-before-boundary.html = PASS
> > +BUGWKGTK : http/tests/multipart/stop-crash.html = PASS
> 
> Why can't we unskip these tests now?

I reviewed all tests, the two that I had marked as not passing above are still failing, and one is timing out. The one that's timing out and invalid-image-data.html will require more investigation, I could not understand why they are broken yet, I'd like to investigate that in a separate ticket if that's OK with you.

The third one, http/tests/multipart/load-last-non-html-frame.php, fails because of a bug in soup's handling of the termination boundary marker, I have a patch up in bugzilla, we will be able to unskip after it's in: http://bugzilla.gnome.org/show_bug.cgi?id=685752

[Gustavo Noronha (kov)]

(In reply to comment #14)
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1020
> > +            d->m_inputStream.leakRef();
> 
> Why do we need to leak this ref here?

That was a mistake, the intention was to .clear() it, fixed!

[Gustavo Noronha (kov)]

(In reply to comment #16)
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-449
> > -    d->m_inputStream = adoptGRef(in);
> 
> This seems to leak 'in' later on in some cases? There are a couple of possible function exit points before 'd->m_inputStream = adoptGRef(in)' is now called.

I made in a GRefPtr itself, that should deal with this.

[Martin Robinson]

Comment on attachment 168025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168025&action=review

Looks good, but I have a remaining question about when you close the multipart input stream.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:378
> +    d->m_soupRequest.clear();
> +
> +    d->m_inputStream.clear();
>  
> -    if (d->m_inputStream)
> -        d->m_inputStream.clear();
> +    d->m_multipartInputStream.clear();
>  
>      d->m_cancellable.clear();

Extreme nit here, but removing the newlines in between these .clear() statements would make this easier for me to read.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:422
> +static void nextPartCallback(GObject* source, GAsyncResult* result, gpointer data)

Perhaps make the name even more explicit: nextMultipartMessagePartCallback?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:435
> +    d->m_inputStream = adoptGRef(soup_multipart_input_stream_next_part_finish(d->m_multipartInputStream.get(), result, &error.outPtr()));

I think it makes sense to add

ASSERT(!d->m_inputStream);

before this line

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:439
> +    if (error) {
> +        client->didFail(handle.get(), ResourceError::httpError(d->m_soupMessage.get(), error.get(), d->m_soupRequest.get()));
> +        cleanupSoupRequestOperation(handle.get());
> +        return;

It's okay not to close m_multipartInputStream in this case?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:457
> +    if (d->m_cancelled || !client) {
> +        cleanupSoupRequestOperation(handle.get());
> +        return;
> +    }

Ditto.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:482
> +    GRefPtr<GInputStream> in = adoptGRef(soup_request_send_finish(d->m_soupRequest.get(), res, &error.outPtr()));

in -> inputStream perhaps?

[Gustavo Noronha (kov)]

Created attachment 168030 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 168030 [details]
Patch

LGTM, but Martin already reviewed it so I'll wait for him to give the final r+

[Jussi Kukkonen (jku)]

Comment on attachment 168030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168030&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:442
> +    if (!d->m_inputStream) {

Testing on EFL (WK2), this never seems to be true. At least multipart-html.php just keeps printing "Downloading: /tmp/multipart-html.php" over and over.

[Gustavo Noronha (kov)]

Jussi, do you mind testing with this soup patch? Thanks everyone for the reviews! https://bugzilla.gnome.org/show_bug.cgi?id=685752

[Gustavo Noronha (kov)]

Created attachment 169232 [details]
Patch

[Jussi Kukkonen (jku)]

(In reply to comment #26)
> Jussi, do you mind testing with this soup patch? Thanks everyone for the reviews! https://bugzilla.gnome.org/show_bug.cgi?id=685752

Oh, did not notice this on monday... 

I still see an apparently endless loop in multipart-html.php on current EFL port with this patch, using libsoup 2d939779 (Be more aggressive when looking for the termination boundary).

I'm happy to debug this but not entirely sure where to look (let's discuss on IRC if you have hints).

[Jussi Kukkonen (jku)]

Just checked the Gtk result, and that does indeed work here. It's just EFL that's failing. This is multipart-html.php result page:
---
Error!

Code: 102
Description: Frame load was interrupted
URL: http://localhost/multipart/multipart-html.php
---
(before that it does print the "This text should only appear once N" text with N up to 8 or so)

At the same time console keeps printing "Downloading: /tmp/multipart-html.php"

[Jussi Kukkonen (jku)]

...and now I notice that that specific test is marked as expected to time out on GTK as well. Maybe the GTK MiniBrowser just does not report the weird error case ?

[Gustavo Noronha (kov)]

(In reply to comment #30)
> ...and now I notice that that specific test is marked as expected to time out on GTK as well. Maybe the GTK MiniBrowser just does not report the weird error case ?

Yeah, the problem only happens on wk2, unsure why right now, in wkgtk it does pass. Are you testing on wk2 efl?

[Jussi Kukkonen (jku)]

(In reply to comment #31)
> (In reply to comment #30)
> > ...and now I notice that that specific test is marked as expected to time out on GTK as well. Maybe the GTK MiniBrowser just does not report the weird error case ?
> 
> Yeah, the problem only happens on wk2, unsure why right now, in wkgtk it does pass. Are you testing on wk2 efl?

Sorry, forgot this. Yes, testing on WK2. 

If the tests are expected to timeout because of other reasons, then we can handle those later in other bugs: I was just under the impression that I was seeing different results on efl than on gtk but that does not seem to be the case.

Also, I can update the efl expectations after this lands.

[Martin Robinson]

*** Bug 54251 has been marked as a duplicate of this bug. ***

[Martin Robinson]

Comment on attachment 169232 [details]
Patch

Let's do this!

[Gustavo Noronha (kov)]

Committed r137150: <http://trac.webkit.org/changeset/137150>