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:
<rdar://problem/17886686>
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>
(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.