Summary: | Sometimes Gmail cannot load messages, particularly on refresh ("...the application ran into an unexpected error...") | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||
Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, beidson, commit-queue, darin, ddkilzer, esprehn+autocc, ggaren, jeffrey+webkit, kangil.han, mark.lam, psolanki, yong.li.webkit | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | iPhone / iPad | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 135882 | ||||||||||
Attachments: |
|
Description
Daniel Bates
2014-08-06 22:09:38 PDT
Created attachment 236173 [details]
Patch
I'm unclear how to write a test for this change without exposing WebCore internals or using a timer with an arbitrary timeout interval to detect if we don't process the list of pending tasks. I'm open to suggestions. Just commenting not reviewing :) I have been assuming that no JS execution is allowed when "defers loading" flag is on. But according to your change log, I'm no longer certain. I would try to find out where JS execution is triggered even when page loading is deferred, and fix the code there. Traditionally and ideally, when a page is deferred, all substitute loading tasks will be deferred (the underlying networking data may be cached, but it shouldn't call back to the loader until "defersLoading" flag is cleared. However, not all developers are aware of this "defersLoading" stuff, so people keep adding code that break this. It seems Safari allows client to call setDefersLoading on a single page and it doesn't suspend active DOM objects. I don't know what this API is supposed to do. Then I'm no longer clear what "defersLoading" means. (In reply to comment #4) > I have been assuming that no JS execution is allowed when "defers loading" flag is on. But according to your change log, I'm no longer certain. I would try to find out where JS execution is triggered even when page loading is deferred, and fix the code there. > > Traditionally and ideally, when a page is deferred, all substitute loading tasks will be deferred (the underlying networking data may be cached, but it shouldn't call back to the loader until "defersLoading" flag is cleared. However, not all developers are aware of this "defersLoading" stuff, so people keep adding code that break this. > Right, Page::setDefersLoading(true) is broken at least with respect to deferring loading of substitute data. (It doesn't defer such loads). We should look to fix this such that setting Page::setDefersLoading(true) defers all loads as its name implies, including loading of substitute data. I haven't thought through the changes required to implement such a fix. For now, I thought it would be sufficient to workaround this issue with the proposed patch. (I'll file a bug for fixing defersLoading for substitute data and update the patch with a FIXME comment that reference the bug). > It seems Safari allows client to call setDefersLoading on a single page and it doesn't suspend active DOM objects. I don't know what this API is supposed to do. Then I'm no longer clear what "defersLoading" means. I'm not familiar with the historical nature of Page::setDefersLoading() or the WebKit API/SPI that uses it. I'll further investigate. Comment on attachment 236173 [details]
Patch
r=me
Created attachment 236201 [details]
Alternative Patch - Make Page::setDefersLoading() defer substitute data load
For completeness, attached an alternative patch that fixes this bug by making Page::setDefersLoading() defer loading of substitute data. Obviously, this patch would change the behavior of WebKit API/SPI (e.g. -[WebView setDefersCallbacks:YES]).
Comment on attachment 236173 [details] Patch Clearing flags on attachment: 236173 Committed r172219: <http://trac.webkit.org/changeset/172219> All reviewed patches have been landed. Closing bug. Reverted r172219 for reason: Caused some /fast/workers tests to fail; will investigate offline. Committed r172225: <http://trac.webkit.org/changeset/172225> It's pretty straightforward to test something like this by adding a test-only API to "window.internals". It's OK for the API to do something very low level. For example, you could expose "window.internals.setDefersLoading(bool)" directly to a purpose-built test. Created attachment 236223 [details] Patch Updated patch whose test results for fast/workers is materially consist with the output of the same tests in a build without the patch and on the build.webkit.org build bots prior to <http://trac.webkit.org/changeset/172219>. Comment on attachment 236223 [details] Patch Clearing flags on attachment: 236223 Committed r172275: <http://trac.webkit.org/changeset/172275> All reviewed patches have been landed. Closing bug. (In reply to comment #11) > It's pretty straightforward to test something like this by adding a test-only API to "window.internals". It's OK for the API to do something very low level. For example, you could expose "window.internals.setDefersLoading(bool)" directly to a purpose-built test. Filed bug #135882 to write a test. |