RESOLVED FIXED 147135
Web Inspector: AppCache manifest 404 doesn't produce errors in console, manifest resource request always loading indicator 
https://bugs.webkit.org/show_bug.cgi?id=147135
Summary Web Inspector: AppCache manifest 404 doesn't produce errors in console, manif...
Joseph Pecoraro
Reported 2015-07-20 19:09:29 PDT
* SUMMARY AppCache manifest 404 doesn't produce errors in console, manifest resource request always loading indicator * STEPS TO REPRODUCE 1. Inspect a page with <html manifest="does-not-exist.manifest"> => no errors in Web Inspector about AppCache manifest failure 2. Reload => manifest request is shown, but never completes, always shows loading indicator
Attachments
[PATCH] Proposed Fix (11.73 KB, patch)
2015-07-20 19:28 PDT, Joseph Pecoraro 		ap: review+
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (12.94 KB, patch)
2015-07-21 11:55 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2015-07-20 19:09:47 PDT
<rdar://problem/21905830>
Joseph Pecoraro
Comment 2 2015-07-20 19:28:56 PDT
Created attachment 257155 [details] [PATCH] Proposed Fix
Alexey Proskuryakov
Comment 3 2015-07-20 20:15:46 PDT
Comment on attachment 257155 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=257155&action=review > LayoutTests/http/tests/appcache/abort-cache-ondownloading-manifest-404-expected.txt:1 > +CONSOLE MESSAGE: Application Cache manifest could not be fetched. I'm worried about the extra logging making the tests flaky. Appcache fetching is very asynchronous, and we already have tests that flakily fail because of that: https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r187025%20(13213)/http/tests/appcache/abort-cache-ondownloading-resource-404-diff.txt https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r187025%20(13213)/http/tests/appcache/abort-cache-onchecking-manifest-404-diff.txt https://build.webkit.org/results/Apple%20Yosemite%20(Leaks)/r187030%20(2399)/http/tests/security/appcache-in-private-browsing-diff.txt What due diligence did you perform? > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:495 > + DocumentLoader* loader = m_frame->loader().documentLoader(); The loader variable doesn't seem to be used again, I'd just put the computation at call site. > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:661 > + m_frame->document()->addConsoleMessage(MessageSource::AppCache, MessageLevel::Error, ASCIILiteral("Application Cache manifest could not be fetched.")); Why not tell the exact reason why? Console messages don't have to be cryptic. > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:671 > + m_frame->document()->addConsoleMessage(MessageSource::AppCache, MessageLevel::Error, ASCIILiteral("Application Cache manifest could not be fetched.")); Ditto (even though it's existing code).
Joseph Pecoraro
Comment 4 2015-07-21 11:46:40 PDT
(In reply to comment #3) > Comment on attachment 257155 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=257155&action=review > > > LayoutTests/http/tests/appcache/abort-cache-ondownloading-manifest-404-expected.txt:1 > > +CONSOLE MESSAGE: Application Cache manifest could not be fetched. > > I'm worried about the extra logging making the tests flaky. Appcache > fetching is very asynchronous, and we already have tests that flakily fail > because of that: > > https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/ > r187025%20(13213)/http/tests/appcache/abort-cache-ondownloading-resource-404- > diff.txt > https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/ > r187025%20(13213)/http/tests/appcache/abort-cache-onchecking-manifest-404- > diff.txt > https://build.webkit.org/results/Apple%20Yosemite%20(Leaks)/r187030%20(2399)/ > http/tests/security/appcache-in-private-browsing-diff.txt > > What due diligence did you perform? That kinda stinks, I'm not sure why that would happen, these logs are synchronous from the operation that caused them. Maybe we should disable CONSOLE MESSAGE logs in these tests? When I run the tests locally they all always pass. When I start --iteration=2 one test times out. When I do --iterations=3 a few tests have issues but I can't get all their failures: shell> ./Tools/Scripts/run-webkit-tests --release --no-retry-failures LayoutTests/http/tests/appcache --iterations=3 ... Found 80 tests; running 78 (3 times each: --repeat-each=1 --iterations=3), skipping 2. Running 1 WebKitTestRunner. [94/234] http/tests/appcache/deferred-events-delete-while-raising.html failed unexpectedly (text diff) [155/234] http/tests/appcache/x-frame-options-prevents-framing.php failed unexpectedly (test timed out, text diff) [173/234] http/tests/appcache/deferred-events.html failed unexpectedly (text diff) 231 tests ran as expected, 3 didn't: ... Regressions: Unexpected text-only failures (1) http/tests/appcache/deferred-events.html [ Failure ] None of the failures however seem to be the new logs I added. Likewise --repeat-each=3 passes all tests but the (http/tests/appcache/x-frame-options-prevents-framing.php) timeout which always seems to happen in iterations=2.
Joseph Pecoraro
Comment 5 2015-07-21 11:55:16 PDT
Created attachment 257189 [details] [PATCH] Proposed Fix Addressed comments.
WebKit Commit Bot
Comment 6 2015-07-22 14:37:24 PDT
Comment on attachment 257189 [details] [PATCH] Proposed Fix Clearing flags on attachment: 257189 Committed r187185: <http://trac.webkit.org/changeset/187185>
WebKit Commit Bot
Comment 7 2015-07-22 14:37:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.