Bug 108380 – REGRESSION(r141136): Apple's internal PLT test suite doesn't finish

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

[Ryosuke Niwa]

Apple's internal PLT test suite doesn't finish after r141136

[Ryosuke Niwa]

Created attachment 185564 [details]
Temporarily disable the feature

[Alexey Proskuryakov]

Comment on attachment 185564 [details]
Temporarily disable the feature

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

r=me

We should make a reproducible case that's not internal.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:482
> +#if PLATFORM(CHROMIUM) || PLATFORM(MAC)
>      // FIXME: Temporarily leave main resource caching disabled for chromium, see https://bugs.webkit.org/show_bug.cgi?id=107962
>      // Ensure main resources aren't preloaded, and other main resource loads are removed from cache to prevent reuse.

Please add a comment about Mac here.

[Ryosuke Niwa]

Committed r141306: <http://trac.webkit.org/changeset/141306>

[Alexey Proskuryakov]

Comment on attachment 185564 [details]
Temporarily disable the feature

Clearing cq+ from a landed patch.

[Radar WebKit Bug Importer]

<rdar://problem/13119725>

[Ryosuke Niwa]

The patch landed here fixed the bug so we need to investigate further and come up with a reduction.

[Ryosuke Niwa]

It appears that WebFrameLoaderClient::dispatchDidFinishLoading or  WebFrameLoaderClient::dispatchDidFailLoading is called with identifier of 0.

[Jessie Berlin]

5 tests started failing on Mountain Lion WK1 Release tests in between 141305 and 141310:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/6313
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r141310%20(6314)/results.html

http/tests/cache/cached-main-resource.html
http/tests/inspector/resource-har-pages.html
http/tests/loading/redirect-methods.html
http/tests/misc/link-rel-icon-beforeload.html
http/tests/misc/favicon-loads-with-images-disabled.html

Especially for the first one, this change seems very suspect

[Nate Chapin]

(In reply to comment #8)
> 5 tests started failing on Mountain Lion WK1 Release tests in between 141305 and 141310:
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/6313
> http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r141310%20(6314)/results.html
> 
> http/tests/cache/cached-main-resource.html
> http/tests/inspector/resource-har-pages.html
> http/tests/loading/redirect-methods.html
> http/tests/misc/link-rel-icon-beforeload.html
> http/tests/misc/favicon-loads-with-images-disabled.html
> 
> Especially for the first one, this change seems very suspect

All of those (except resource-har-pages.html) are expected to change, given that their results depend on the feature disabled in r141306. It's probably best to just expect them to fail until main resource caching is re-enabled.

[Jessie Berlin]

(In reply to comment #9)
> (In reply to comment #8)
> > 5 tests started failing on Mountain Lion WK1 Release tests in between 141305 and 141310:
> > http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/6313
> > http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r141310%20(6314)/results.html
> > 
> > http/tests/cache/cached-main-resource.html
> > http/tests/inspector/resource-har-pages.html
> > http/tests/loading/redirect-methods.html
> > http/tests/misc/link-rel-icon-beforeload.html
> > http/tests/misc/favicon-loads-with-images-disabled.html
> > 
> > Especially for the first one, this change seems very suspect
> 
> All of those (except resource-har-pages.html) are expected to change, given that their results depend on the feature disabled in r141306. It's probably best to just expect them to fail until main resource caching is re-enabled.

Marked them as expected failures in platform/mac’s TestExpectations:
http://trac.webkit.org/changeset/141469

[Jessie Berlin]

Actually, http/tests/inspector/resource-har-pages.html was changed in the checkin that enabled this feature:

http://trac.webkit.org/changeset/141136

I am going to mark it as failing for now as well.

[Nate Chapin]

Has anyone had a chance to test whether this suite still fails since http://trac.webkit.org/changeset/141615 landed (with main resource caching re-enabled)?

[Ryosuke Niwa]

(In reply to comment #12)
> Has anyone had a chance to test whether this suite still fails since http://trac.webkit.org/changeset/141615 landed (with main resource caching re-enabled)?

Yes, r141615 fixed the PLT when main resource caching is enabled. My plan is to post a new patch that reverts r141306. Sorry I got distracted by other work.

[Nate Chapin]

(In reply to comment #13)
> (In reply to comment #12)
> > Has anyone had a chance to test whether this suite still fails since http://trac.webkit.org/changeset/141615 landed (with main resource caching re-enabled)?
> 
> Yes, r141615 fixed the PLT when main resource caching is enabled. My plan is to post a new patch that reverts r141306. Sorry I got distracted by other work.

No problem, I just wanted to make sure there weren't other regressions blocking re-enable :)

[Ryosuke Niwa]

Created attachment 186702 [details]
Re-enable the reuse of main resource cache

[Ryosuke Niwa]

I'm going to wait for EWS bots to catch up.

[WebKit Review Bot]

Comment on attachment 186702 [details]
Re-enable the reuse of main resource cache

Clearing flags on attachment: 186702

Committed r142024: <http://trac.webkit.org/changeset/142024>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.