92300 2012-07-25 14:24:36 -0700 [BlackBerry] Rephrase suspend/resume condition to guard against crashes 2012-07-26 07:47:41 -0700 1 1 1 Unclassified WebKit WebKit BlackBerry 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jpetsovits jpetsovits anilsson manyoso mifenton staikos tonikitoo webkit.review.bot oldest_to_newest 678467 0 jpetsovits 2012-07-25 14:24:36 -0700 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. 678472 1 154445 jpetsovits 2012-07-25 14:27:58 -0700 Created attachment 154445 Patch 678577 2 154445 webkit.review.bot 2012-07-25 16:15:20 -0700 Comment on attachment 154445 Patch Clearing flags on attachment: 154445 Committed r123673: <http://trac.webkit.org/changeset/123673> 678578 3 webkit.review.bot 2012-07-25 16:15:24 -0700 All reviewed patches have been landed. Closing bug. 678879 4 anilsson 2012-07-25 22:58:55 -0700 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. 679004 5 anilsson 2012-07-26 01:16:27 -0700 (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. 679016 6 anilsson 2012-07-26 01:25:36 -0700 (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. 679143 7 anilsson 2012-07-26 04:24:06 -0700 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... 679144 8 anilsson 2012-07-26 04:27:15 -0700 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(); 679201 9 anilsson 2012-07-26 05:33:30 -0700 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 =) 679276 10 tonikitoo 2012-07-26 06:49:02 -0700 (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... :( 679325 11 jpetsovits 2012-07-26 07:47:41 -0700 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. 154445 2012-07-25 14:27:58 -0700 2012-07-25 16:15:20 -0700 Patch bug-92300-20120725172655.patch text/plain 4028 jpetsovits U3VidmVyc2lvbiBSZXZpc2lvbjogMTIzNjEwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Js YWNrYmVycnkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0NoYW5nZUxvZwpp bmRleCA1MWU0NGFmM2M5YWRhZjA3N2M3YzU0ODAzY2ZkMTYxNmQyNjU0NTcyLi5jZTY0MzgwNDgy MjMxNzY2MTZlZmE5NDkwNzY4ZWI5MzI1ZjkyMWUwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L2JsYWNrYmVycnkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFu Z2VMb2cKQEAgLTEsNSArMSwzOCBAQAogMjAxMi0wNy0yNSAgSmFrb2IgUGV0c292aXRzICA8anBl dHNvdml0c0ByaW0uY29tPgogCisgICAgICAgIFtCbGFja0JlcnJ5XSBSZXBocmFzZSBzdXNwZW5k L3Jlc3VtZSBjb25kaXRpb24gdG8gZ3VhcmQgYWdhaW5zdCBjcmFzaGVzCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05MjMwMAorICAgICAgICBQUiAxODIx MjUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXaGVu IHRoZSBjb21wb3NpdG9yIGlzIHJlbW92ZWQgZnJvbSBhIFdlYlBhZ2UsIGl0IG1pZ2h0CisgICAg ICAgIGFscmVhZHkgaGF2ZSB1bnNldCBpdHMgY29udGV4dCwgYW5kIGlzc3VpbmcgYSBjYWxsIHRv CisgICAgICAgIEJhY2tpbmdTdG9yZTo6YnVmZmVyKCkgd291bGQgdGhlcmVmb3JlIGNhdXNlIGEg Y3Jhc2guCisgICAgICAgIEp1c3QgZ3VhcmRpbmcgYnVmZmVyKCkgZnJvbSB0aGlzIGNyYXNoIGlz bid0IGZ1bGx5CisgICAgICAgIGNvcnJlY3QsIGJlY2F1c2UgdGhlbiB3ZSB3b3VsZG4ndCBzdXNw ZW5kIHJlbmRlcmluZworICAgICAgICBmb3IgYSBjb21wb3NpdG9yIHRoYXQgaGFkIGJlZW4gcHJl dmlvdXNseSBlbmFibGVkLgorCisgICAgICAgIEluc3RlYWQsIGNoYW5nZSB0aGUgY29uZGl0aW9u IHRvIHN1c3BlbmQvcmVzdW1lIGluCisgICAgICAgIFdlYlBhZ2VQcml2YXRlOjpzZXRDb21wb3Np dG9yKCkgdG8gZm9jdXMgb24gdGhlCisgICAgICAgIG9iamVjdCB0aGF0IGRlbGl2ZXJzIHVzIHRo ZSBidWZmZXIuIElmIHdlIGNhbid0CisgICAgICAgIGxpc3RlbiB0byB0aGUgY29udGV4dCBvciBi dWZmZXIgYmVpbmcgc2V0LCBiZXR0ZXIKKyAgICAgICAganVzdCB0YWtlIHRoZSBjb21wb3NpdG9y IG9iamVjdCBpdHNlbGYgdG8gZGV0ZXJtaW5lCisgICAgICAgIHdoZXRoZXIgd2UgaGF2ZSBzb21l dGhpbmcgdmFsaWQgb3Igbm90Li4uIGFuZCBob3BlCisgICAgICAgIHRoYXQgdGhleSBnaXZlIHVz IGEgdmFsaWQgY29udGV4dCAmIGJ1ZmZlciBpbiBhbGwKKyAgICAgICAgc2l0dWF0aW9ucyB3aGVu IHdlIGNhbiBhY3R1YWxseSBiZSByZW5kZXJpbmcuCisKKyAgICAgICAgQWxzbyBjaGVjayBjb21w b3NpdG9yLT5jb250ZXh0KCkgaW4gYnVmZmVyKCkgdG8gYmUKKyAgICAgICAgbm9uLXplcm8gYmVm b3JlIGFjY2Vzc2luZyB0aGUgY29udGV4dCdzIGJ1ZmZlciwKKyAgICAgICAgYmVjYXVzZSBtb3Jl IGRlZmVuc2l2ZSBjb2RpbmcgY2FuJ3QgaHVydCBoZXJlLgorCisgICAgICAgICogQXBpL0JhY2tp bmdTdG9yZS5jcHA6CisgICAgICAgIChCbGFja0JlcnJ5OjpXZWJLaXQ6OkJhY2tpbmdTdG9yZVBy aXZhdGU6OmJ1ZmZlcik6CisgICAgICAgICogQXBpL1dlYlBhZ2UuY3BwOgorICAgICAgICAoQmxh Y2tCZXJyeTo6V2ViS2l0OjpXZWJQYWdlUHJpdmF0ZTo6c2V0Q29tcG9zaXRvcik6CisKKzIwMTIt MDctMjUgIEpha29iIFBldHNvdml0cyAgPGpwZXRzb3ZpdHNAcmltLmNvbT4KKwogICAgICAgICBb QmxhY2tCZXJyeV0gbm90aWZ5Q29udGVudFJlbmRlcmVkKCkgY2FsbCBtaXNzaW5nIGluIHR3byBz cG90cwogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTIx NTMKICAgICAgICAgUklNIFBSIDE3MzM0MApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9ibGFj a2JlcnJ5L0FwaS9CYWNraW5nU3RvcmUuY3BwIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0Fw aS9CYWNraW5nU3RvcmUuY3BwCmluZGV4IGFhOTI3NTUzOGY0MDYyNjg0YzJhMzJhNDJkOThmNTg0 MTk0M2RhOGMuLmQ5YjFjZmNjMGMwYjUyYjc1MTNjZWI3MTkwMzMwYjMyODM5MWVmMzkgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvQmFja2luZ1N0b3JlLmNwcAorKysg Yi9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvQXBpL0JhY2tpbmdTdG9yZS5jcHAKQEAgLTI4Nzgs NyArMjg3OCw3IEBAIFBsYXRmb3JtOjpHcmFwaGljczo6QnVmZmVyKiBCYWNraW5nU3RvcmVQcml2 YXRlOjpidWZmZXIoKSBjb25zdAogCiAjaWYgVVNFKEFDQ0VMRVJBVEVEX0NPTVBPU0lUSU5HKQog ICAgIGlmIChXZWJQYWdlQ29tcG9zaXRvclByaXZhdGUqIGNvbXBvc2l0b3IgPSBtX3dlYlBhZ2Ut PmQtPmNvbXBvc2l0b3IoKSkKLSAgICAgICAgcmV0dXJuIGNvbXBvc2l0b3ItPmNvbnRleHQoKS0+ YnVmZmVyKCk7CisgICAgICAgIHJldHVybiBjb21wb3NpdG9yLT5jb250ZXh0KCkgPyBjb21wb3Np dG9yLT5jb250ZXh0KCktPmJ1ZmZlcigpIDogMDsKICNlbmRpZgogCiAgICAgcmV0dXJuIDA7CmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvQXBpL1dlYlBhZ2UuY3BwIGIvU291 cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlLmNwcAppbmRleCAxYjM0NmYyMWZmMTc4 NmQ3YmUzZmYyNGU2NmM2YmU4NTRjNTQyZGI5Li5iMjVmZThiZjg3NDlhYWUyOTRhNzI0NTNjMjQ5 ODJiZjE0ZDNmZGQ3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvQXBpL1dl YlBhZ2UuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAK QEAgLTU5MjksNyArNTkyOSw3IEBAIHZvaWQgV2ViUGFnZVByaXZhdGU6OnNldENvbXBvc2l0b3Io UGFzc1JlZlB0cjxXZWJQYWdlQ29tcG9zaXRvclByaXZhdGU+IGNvbXBvc2l0CiAgICAgLy8gVGhh dCBzZWVtcyBleHRyZW1lbHkgbGlrZWx5IHRvIGJlIHRoZSBjYXNlLCBidXQgbGV0J3MgYXNzZXJ0 IGp1c3QgdG8gbWFrZSBzdXJlLgogICAgIEFTU0VSVCh3ZWJLaXRUaHJlYWRNZXNzYWdlQ2xpZW50 KCktPmlzQ3VycmVudFRocmVhZCgpKTsKIAotICAgIGlmIChtX2JhY2tpbmdTdG9yZS0+ZC0+YnVm ZmVyKCkpCisgICAgaWYgKG1fY29tcG9zaXRvciB8fCBtX2NsaWVudC0+d2luZG93KCkpCiAgICAg ICAgIG1fYmFja2luZ1N0b3JlLT5kLT5zdXNwZW5kU2NyZWVuQW5kQmFja2luZ1N0b3JlVXBkYXRl cygpOwogCiAgICAgLy8gVGhpcyBtZXRob2QgY2FsbCBhbHdheXMgcm91bmQtdHJpcHMgb24gdGhl IFdlYktpdCB0aHJlYWQgKHNlZSBXZWJQYWdlQ29tcG9zaXRvcjo6V2ViUGFnZUNvbXBvc2l0b3Io KSBhbmQgfldlYlBhZ2VDb21wb3NpdG9yKCkpLApAQCAtNTk0MSw3ICs1OTQxLDcgQEAgdm9pZCBX ZWJQYWdlUHJpdmF0ZTo6c2V0Q29tcG9zaXRvcihQYXNzUmVmUHRyPFdlYlBhZ2VDb21wb3NpdG9y UHJpdmF0ZT4gY29tcG9zaXQKICAgICAvLyBzYWZlIGFjY2VzcyB0byBtX2NvbXBvc2l0b3IgYW5k IGl0cyByZWZjb3VudC4KICAgICB1c2VySW50ZXJmYWNlVGhyZWFkTWVzc2FnZUNsaWVudCgpLT5k aXNwYXRjaFN5bmNNZXNzYWdlKGNyZWF0ZU1ldGhvZENhbGxNZXNzYWdlKCZXZWJQYWdlUHJpdmF0 ZTo6c2V0Q29tcG9zaXRvckhlbHBlciwgdGhpcywgY29tcG9zaXRvciwgY29tcG9zaXRpbmdDb250 ZXh0KSk7CiAKLSAgICBpZiAobV9iYWNraW5nU3RvcmUtPmQtPmJ1ZmZlcigpKSAvLyB0aGUgbmV3 IGNvbXBvc2l0b3IsIGlmIG9uZSB3YXMgc2V0CisgICAgaWYgKG1fY29tcG9zaXRvciB8fCBtX2Ns aWVudC0+d2luZG93KCkpIC8vIHRoZSBuZXcgY29tcG9zaXRvciwgaWYgb25lIHdhcyBzZXQKICAg ICAgICAgbV9iYWNraW5nU3RvcmUtPmQtPnJlc3VtZVNjcmVlbkFuZEJhY2tpbmdTdG9yZVVwZGF0 ZXMoQmFja2luZ1N0b3JlOjpSZW5kZXJBbmRCbGl0KTsKIH0KIAo=