[EFL] REGRESSION(r173394): MiniBrowser stucked in an infinite loop if NETWORK_CACHE is disabled

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Tibor Mészáros]

MiniBrowser stucked in an infinite loop on http://index.hu if NetworkProcess is enabled

The new caching method introduced in r173394 causes this problem on EFL.
You can easily reproduce it with:

MiniBrowser http://index.hu -S

[Tibor Mészáros]

Created attachment 239798 [details]
WIP patch

If we disable caching, or reduce the caching time, everything works fine. But I started to debug the real bug and try to fix it properly.

[Csaba Osztrogonác]

Change importance to P1/blocker, because NetworkProcess is mandatory
since http://trac.webkit.org/changeset/192796 and now networking is
completely broken on EFL WebKit:

https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/25715

Any volunteer to pick up this serious regression?

[Csaba Osztrogonác]

just to note: https://trac.webkit.org/changeset/192994 skipped all wpt tests.
They should be unskipped once somebody fixes this very old regression.

[Gyuyoung Kim]

(In reply to comment #3)
> just to note: https://trac.webkit.org/changeset/192994 skipped all wpt tests.
> They should be unskipped once somebody fixes this very old regression.

To fix it, I file a new bug - https://bugs.webkit.org/show_bug.cgi?id=151801

[Csaba Osztrogonác]

(In reply to comment #4)
> (In reply to comment #3)
> > just to note: https://trac.webkit.org/changeset/192994 skipped all wpt tests.
> > They should be unskipped once somebody fixes this very old regression.
> 
> To fix it, I file a new bug - https://bugs.webkit.org/show_bug.cgi?id=151801

Common, why do we need a new bug report for the same bug?

REGRESSION(r192994): imported/w3c/web-platform-tests has been timeout since r192994
https://bugs.webkit.org/show_bug.cgi?id=151801

Otherwise the name of the new bug is misleading, because r192994 only
made NetworkProcess mandatory, it isn't the real culprit. It is a 15
months old bug caused by r173394. But it was hidden, because EFL
didn't use NetworkProcess at all until it became mandatory.


https://bugs.webkit.org/show_bug.cgi?id=151801#c2 pointed out
that the problem occurs only if NETWORK_CACHE is disabled.

So one possible fix can be to enable NETWORK_CACHE on EFL port too
similar to Apple Mac and GTK port. In this case can we get rid
NETWORK_CACHE guards since all ports enabled it? And we see that
networking is broken with disabled NETWORK_CACHE long time ago.
Is there any reason to keep this broken configuration in the tree?

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

@Antti and @Darin: You are the author and the reviewer of r173394 which
introduced this regression. What do you think, what would be the proper fix?

Should we try to fix the broken !NETWORK_CACHE 
configuration or should we remove it completely?

[Darin Adler]

I think that roughly speaking someone can put:

#if !NETWORK_CACHE
    return 0_ms;
#endif

in the maximumBufferingTime function. That will turn off the caching Antti’s patch introduced.

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > just to note: https://trac.webkit.org/changeset/192994 skipped all wpt tests.
> > > They should be unskipped once somebody fixes this very old regression.
> > 
> > To fix it, I file a new bug - https://bugs.webkit.org/show_bug.cgi?id=151801
> 
> Common, why do we need a new bug report for the same bug?
> 
> REGRESSION(r192994): imported/w3c/web-platform-tests has been timeout since
> r192994
> https://bugs.webkit.org/show_bug.cgi?id=151801
> 
> Otherwise the name of the new bug is misleading, because r192994 only
> made NetworkProcess mandatory, it isn't the real culprit. It is a 15
> months old bug caused by r173394. But it was hidden, because EFL
> didn't use NetworkProcess at all until it became mandatory.

For the record, test_ewk2_context_menu has been failing since r192796.
- https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/25715

Let's see if this broken test can be fixed as well.

[Csaba Osztrogonác]

Created attachment 266876 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #10)
> Created attachment 266876 [details]
> Patch

(In reply to comment #8)
> I think that roughly speaking someone can put:
> 
> #if !NETWORK_CACHE
>     return 0_ms;
> #endif
> 
> in the maximumBufferingTime function. That will turn off the caching Antti’s
> patch introduced.

Thanks for the suggestions, it seems it works fine.

[Darin Adler]

Comment on attachment 266876 [details]
Patch

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

> Source/WebKit2/WebProcess/Network/WebLoaderStrategy.cpp:91
> +#if !ENABLE(NETWORK_CACHE)
> +    return 0_ms;
> +#endif

I think we need a comment explaining why we are doing this. It’s a mysterious line of code!

[WebKit Commit Bot]

Comment on attachment 266876 [details]
Patch

Clearing flags on attachment: 266876

Committed r193752: <http://trac.webkit.org/changeset/193752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.