Bug 135688 – Sometimes Gmail cannot load messages, particularly on refresh ("...the application ran into an unexpected error...")

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

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:

[Daniel Bates]

<rdar://problem/17886686>

[Daniel Bates]

Created attachment 236173 [details]
Patch

[Daniel Bates]

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]

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]

(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 on attachment 236173 [details]
Patch

r=me

[Daniel Bates]

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 on attachment 236173 [details]
Patch

Clearing flags on attachment: 236173

Committed r172219: <http://trac.webkit.org/changeset/172219>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Reverted r172219 for reason:

Caused some /fast/workers tests to fail; will investigate offline.

Committed r172225: <http://trac.webkit.org/changeset/172225>

[Geoffrey Garen]

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]

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 on attachment 236223 [details]
Patch

Clearing flags on attachment: 236223

Committed r172275: <http://trac.webkit.org/changeset/172275>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(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.