Bug 135688

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 Flags
Patch
none
Alternative Patch - Make Page::setDefersLoading() defer substitute data load
none
Patch none

Daniel Bates
Reported 2014-08-06 22:09:38 PDT
On iOS, sometimes Gmail cannot load the list of messages. This visually manifests itself with Gmail showing a spinner icon and subsequently showing an error message after approximately 1 minute with the following text: "Oops, the application ran into an unexpected error. Please refresh this page in your browser and try again." This bug is difficult to reproduce. Perform the following steps, stopping at the step that first reproduces this bug: 1. Open Safari and navigate to gmail.com and sign in. 2. Force quit Safari. 3. Open Safari and let it load gmail.com. 4. If the list of messages appears then tap the reload button in the browser. 5. If the list of messages appears then close the tab, open a new tab and navigate to gmail.com. 6. If the list of messages appears then repeat from step 4. Additional remark: This bug is a caused by a side effect of the patch for bug #49401. Bug #84488 was filed to address the side effects of bug #49401 via refactoring of the page load deferrer code. I'm breaking out the issue of tasks placed into the pending task queue when loading is deferred from bug #84488, comment 0, from the larger refactoring effort of that bug as it seems sufficient address such a side effect in a separate bug (this bug). Reprinting the relevant remark from bug #84488, comment 0, here for convenience: [[ One example is Document::[postTask() (http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp?rev=171886#L4908)]. If page->defersLoading() is true, it throws the task on a pending queue. But that queue is only flushed when a PageGroupLoadDeferrer is destroyed, and not when the state of page->defersLoading() otherwise changes. This was introduced in [<http://trac.webkit.org/changeset/102278>] and caused a lot of bizarre little side effects. ]] It seems sufficient to only enqueue tasks in the pending task queue of a document when the active DOM objects of the document are active. Reprinting the relevant remark from bug #84488, comment 0, here for convenience:
Attachments
Patch (3.04 KB, patch)
2014-08-06 22:15 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Alternative Patch - Make Page::setDefersLoading() defer substitute data load (9.19 KB, patch)
2014-08-07 11:26 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Patch (3.00 KB, patch)
2014-08-07 14:30 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2014-08-06 22:10:26 PDT
<rdar://problem/17886686>
Daniel Bates
Comment 2 2014-08-06 22:15:37 PDT
Created attachment 236173 [details] Patch
Daniel Bates
Comment 3 2014-08-06 22:19:35 PDT
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.
Yong Li
Comment 4 2014-08-06 23:23:58 PDT
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.
Daniel Bates
Comment 5 2014-08-07 00:31:33 PDT
(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.
Maciej Stachowiak
Comment 6 2014-08-07 11:10:58 PDT
Comment on attachment 236173 [details] Patch r=me
Daniel Bates
Comment 7 2014-08-07 11:26:44 PDT
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]).
Daniel Bates
Comment 8 2014-08-07 11:47:13 PDT
Comment on attachment 236173 [details] Patch Clearing flags on attachment: 236173 Committed r172219: <http://trac.webkit.org/changeset/172219>
Daniel Bates
Comment 9 2014-08-07 11:47:17 PDT
All reviewed patches have been landed. Closing bug.
Daniel Bates
Comment 10 2014-08-07 12:44:50 PDT
Reverted r172219 for reason: Caused some /fast/workers tests to fail; will investigate offline. Committed r172225: <http://trac.webkit.org/changeset/172225>
Geoffrey Garen
Comment 11 2014-08-07 13:48:58 PDT
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.
Daniel Bates
Comment 12 2014-08-07 14:30:03 PDT
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>.
Daniel Bates
Comment 13 2014-08-07 14:39:39 PDT
Comment on attachment 236223 [details] Patch Clearing flags on attachment: 236223 Committed r172275: <http://trac.webkit.org/changeset/172275>
Daniel Bates
Comment 14 2014-08-07 14:39:45 PDT
All reviewed patches have been landed. Closing bug.
Daniel Bates
Comment 15 2014-08-13 08:52:05 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.