Bug 92300 – [BlackBerry] Rephrase suspend/resume condition to guard against crashes

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jakob Petsovits]

In short, we're getting a crash when the WebPageCompositor is set to 0 and its context is 0 too. Fixing the crash is easy, I hope this patch accomplishes it in the most correct way possible. Patch below.

[Jakob Petsovits]

Created attachment 154445 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154445 [details]
Patch

Clearing flags on attachment: 154445

Committed r123673: <http://trac.webkit.org/changeset/123673>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arvid Nilsson]

I would have used "if (m_compositor->context() || m_client->window())" (going from memory, not sure if this is right?).

My thinking is that it's actually the platform::gles2context that provides the buffer. There might be an internal m_compositor due to AC layers. And we might do unbalanced suspend.

[Arvid Nilsson]

(In reply to comment #4)
> I would have used "if (m_compositor->context() || m_client->window())" (going from memory, not sure if this is right?).
> 
> My thinking is that it's actually the platform::gles2context that provides the buffer. There might be an internal m_compositor due to AC layers. And we might do unbalanced suspend.

Jakob correctly pointed out to me that the reason we're here in the first place is that m_compositor->context() can disappear without notice. So that would be a bad idea.

[Arvid Nilsson]

(In reply to comment #5)
> (In reply to comment #4)
> > I would have used "if (m_compositor->context() || m_client->window())" (going from memory, not sure if this is right?).
> > 
> > My thinking is that it's actually the platform::gles2context that provides the buffer. There might be an internal m_compositor due to AC layers. And we might do unbalanced suspend.
> 
> Jakob correctly pointed out to me that the reason we're here in the first place is that m_compositor->context() can disappear without notice. So that would be a bad idea.

The reason I brought this up is I thought somebody had landed the patch to suspend the backingstore if there's no window at the time the WebPage is created, but nobody seems to be doing that yet...

I was thinking of a scenario where there's no window, we're suspended, and we set the external compositor, and it should resume.

But it seems we're not initially suspended after all, window or no window.

[Arvid Nilsson]

Alright, Jakob showed me the initial suspend is in BackingStore::createSurfaces(). This was introduced in fix for bug #91686.

From the commit message:

"   So, in effect, with this patch applied, the sequence
    of events will look like this:
    
    1) WebPage & BackingStore are initialize, neither window
       nor compositor exists, therefore buffer() returns 0.
       createSurface() therefore suspends screen and
       backingstore.
    2) loadURL() or loadData() is called, web page is
       fully loaded, however we don't try to render because
       we're still suspended, still have no target buffer.
    3) A WebPageCompositor is being set from outside.
       At the beginning of WebPage::setCompositor() we still
       don't have a buffer() so there's nothing to suspend,
       however, after the sync call to setCompositorHelper()
       the compositor is set so buffer() will return a
       nonzero value, causing us to resume at this point."

The problem introduced by this patch is that the WebPage::m_compositor is initialized to an internal compositor in WebPagePrivate::initialize:

#if USE(ACCELERATED_COMPOSITING)
    // The compositor will be needed for overlay rendering, so create it
    // unconditionally. It will allocate OpenGL objects lazily, so this incurs
    // no overhead in the unlikely case where the compositor is not needed.
    Platform::userInterfaceThreadMessageClient()->dispatchSyncMessage(
            createMethodCallMessage(&WebPagePrivate::createCompositor, this));
#endif

so m_compositor is non-0 at this point:

    if (m_compositor || m_client->window())
        m_backingStore->d->suspendScreenAndBackingStoreUpdates();

so it will suspend again, and

    if (m_compositor || m_client->window()) // the new compositor, if one was set
        m_backingStore->d->resumeScreenAndBackingStoreUpdates(BackingStore::RenderAndBlit);

will not balance the suspend count...

[Arvid Nilsson]

A possible fix could be

#if USE(ACCELERATED_COMPOSITING)
    // If there's no window, the client will provide a compositor
    if (m_client->window()) {
        // The compositor will be needed for overlay rendering, so create it
        // unconditionally. It will allocate OpenGL objects lazily, so this incurs
        // no overhead in the unlikely case where the compositor is not needed.
        Platform::userInterfaceThreadMessageClient()->dispatchSyncMessage(
                createMethodCallMessage(&WebPagePrivate::createCompositor, this));
    }
#endif

Then the checks in this patch will be valid:

so m_compositor is actually 0 at this point:

    if (m_compositor || m_client->window())
        m_backingStore->d->suspendScreenAndBackingStoreUpdates();

[Arvid Nilsson]

D'oh:

bool WebPagePrivate::createCompositor()
{
    // If there's no window, the compositor will be supplied by the API client
    if (!m_client->window())
        return false;

It already contains that line, but when the message is received, not when it's sent =)

[Antonio Gomes]

(In reply to comment #7)
> Alright, Jakob showed me the initial suspend is in BackingStore::createSurfaces(). This was introduced in fix for bug #91686.
> 
> From the commit message:
...
> 
> The problem introduced by this patch is that the WebPage::m_compositor is initialized to an internal compositor in WebPagePrivate::initialize:

that was exactly my concern, and Jakob and I had a almost long conversation on IRC.

IMO, we seem to be piling up hacks. Suspending from one place, and relying on conditions to resume it elsewhere by making use of a count-based mechanism is *fragile*.

in a count-based suspend/resume mechanism, each suspend has to match exactly a call to resume without relying on any variable or state, in my mind. Otherwise it can/will fail in a given point, and it is going to be hard to find out where...

:(

[Jakob Petsovits]

Oh wow, big discussion here. Sorry for being late to the party. Antonio's and Arvid's latest verdicts are correct, it's unnecessarily fragile but right now there's no easy way around this unless we make the suspend/resume mechanism into more of a state machine. However for a state machine we need to be able to intercept changing states, which right now is difficult since several objects are provided by outside containers but we don't get to know about when they change. We should try to work more towards setter methods in WebPage or BackingStore, rather than using getters from "client"-type objects.

So what Arvid and Antonio are saying is right, I just wasn't in a position to spend more time for a large-scale solution to this issue.