Bug 75288 – [chromium] Prevent crashing due to NULL texture updater.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Penner]

[chromium] Prevent crashing due to NULL texture updater.

[Eric Penner]

Created attachment 120628 [details]
Patch

[Eric Penner]

I believe this is a knock-on from:
https://bugs.webkit.org/show_bug.cgi?id=74477

Actually, just now I looked at the comments and it looks like this code (in a different place) was in the first patch, but then it got removed in the second patch. Anyway, I believe this is the cause for crashing in TiledLayerChromium::createTile on canary.

[Eric Penner]

Hmm, on second thought. This doesn't address the ImageLayerChromium case when the texture updater is accessed earlier...  Perhaps we should get rid of the cleanupResources method and use setLayerTreeHost(NULL) to accomplish this?

[James Robinson]

Comment on attachment 120628 [details]
Patch

clearing review flag since it seems this isn't the right fix

[Eric Penner]

Created attachment 120962 [details]
Patch

[Eric Penner]

Updated so it fixes the crash in all possible cases. There might be a better way to do this but maybe we can create a separate bug for that cleanup?

On a related note, I got OSX building yesterday so I will take a look at why we are so low on memory there. It seems like there is two issues there.

[James Robinson]

When is this happening? The current code creates a textureUpdater whenever the CCLayerTreeHost pointer is set to something non-null. How are we getting to createTile() with a null updater?

Can you make a test?

[Eric Penner]

Updated so it fixes the crash in all possible cases. There might be a better way to do this but maybe we can create a separate bug for that cleanup?

On a related note, I got OSX building yesterday so I will take a look at why we are so low on memory there. It seems like there is two issues there. (In reply to comment #7)
> When is this happening? The current code creates a textureUpdater whenever the CCLayerTreeHost pointer is set to something non-null. How are we getting to createTile() with a null updater?
> 
> Can you make a test?

TiledLayerChromium::prepareToUpdateTiles calls cleanupResources when it runs out of memory, which clears the texture updater. Hmm, I think I can modify an existing test to exercise this condition. Will have that in a sec.

[Eric Penner]

Actually I can't find a place to test this. Similar tests are in TiledLayerChromiumTest, but this fix relies on derived classes, so a test there would be contrived. Hmm..

[Eric Penner]

As another possible option. I'm not sure we really need to call cleanupResources when a TiledLayerChromium runs out of memory. As the user scrolls, a big layer can thrash between failing and succeeding (as it straddles different numbers of tiles), resulting in latter small layers flickering in and out as they fit into the sporadic free memory. I'd rather just accept we can't render the page and avoid the flickering and thrashing.

[Adrienne Walker]

(In reply to comment #8)
 
> TiledLayerChromium::prepareToUpdateTiles calls cleanupResources when it runs out of memory, which clears the texture updater. Hmm, I think I can modify an existing test to exercise this condition. Will have that in a sec.

I don't really understand why the texture updater needs to be cleared in cleanupResources.  Can we just remove ContentLayerChromium::cleanupResources and ImageLayerChromium::cleanupResources()?

[Eric Penner]

Created attachment 120998 [details]
Patch

[James Robinson]

Comment on attachment 120998 [details]
Patch

Seems OK

[WebKit Review Bot]

Comment on attachment 120998 [details]
Patch

Rejecting attachment 120998 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ng file Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h
Hunk #1 FAILED at 56.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h.rej
patching file Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp
patching file Source/WebCore/platform/graphics/chromium/ImageLayerChromium.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/10954686

[Eric Penner]

Created attachment 121035 [details]
rebase

[WebKit Review Bot]

Comment on attachment 121035 [details]
rebase

Clearing flags on attachment: 121035

Committed r104003: <http://trac.webkit.org/changeset/104003>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.