631585 - Leaking ViewWrappers during crashtest/reftest-ipc, from content process

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

REFTEST INFO | runreftest.py | Running tests: end.
crashtest-ipc failed:
TEST-UNEXPECTED-FAIL | tab process 8625 | automationutils.processLeakLog() | leaked 352 bytes during test execution
TEST-UNEXPECTED-FAIL | tab process 8625 | automationutils.processLeakLog() | leaked 11 instances of ViewWrapper with size 32 bytes each (352 bytes total)

Not sure where that's coming from, will bisect over the manifest.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

One leak is from content/media/test/crashtests/459439-1.html.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

... by which I meant HTTP load 481136-1.html.  (Messed up the bisect.)

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sigh.  The leak is timing-dependent, and appears to be coming from nsObjectFrame::CreateWidget.  We shouldn't be hitting that code in content processes anyway; it's just going to cause problems, since content processes can't use native widgets.  I'll ponder a "fix", which might be just returning null.

Also, this test has <object data='sound.ogg'>, which sure doesn't need a widget but I guess we don't find that out until later in the pipeline.  Might be worth fixing separately (delay widget creation until we know we need one).

Comment 4

[Timothy Nikkel (:tnikkel)]

I didn't think we created widgets until we knew that we needed them. Do you have the stack for the creation of the useless widget?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Breakpoint 1, ViewWrapper::ViewWrapper (this=0x16a0c40, aView=0x1653800) at /home/cjones/mozilla/mozilla-central/view/src/nsView.cpp:70
#0  ViewWrapper::ViewWrapper (this=0x16a0c40, aView=0x1653800) at /home/cjones/mozilla/mozilla-central/view/src/nsView.cpp:70
#1  0x00002baa0bc89963 in nsIView::AttachWidgetEventHandler (this=0x1653800, aWidget=0x167f180) at /home/cjones/mozilla/mozilla-central/view/src/nsView.cpp:938
#2  0x00002baa0b702da1 in nsObjectFrame::CreateWidget (this=0x15afc68, aWidth=0, aHeight=0, aViewOnly=0) at /home/cjones/mozilla/mozilla-central/layout/generic/nsObjectFrame.cpp:789
#3  0x00002baa0b7109bd in nsPluginInstanceOwner::CreateWidget (this=0x16b4600) at /home/cjones/mozilla/mozilla-central/layout/generic/nsObjectFrame.cpp:6274
#4  0x00002baa0c5957f4 in nsPluginHost::DoInstantiateEmbeddedPlugin (this=0x1593110, aMimeType=0x15a54c8 "audio/ogg", aURL=0x163be00, aOwner=0x16b4600, aAllowOpeningStreams=0) at /home/cjones/mozilla/mozilla-central/modules/plugin/base/src/nsPluginHost.cpp:1102
#5  0x00002baa0c58d641 in nsPluginStreamListenerPeer::OnStartRequest (this=0x14ff850, request=0x163c328, aContext=0x0) at /home/cjones/mozilla/mozilla-central/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp:604
#6  0x00002baa0b9ab79d in nsObjectLoadingContent::OnStartRequest (this=0x16280a0, aRequest=0x163c328, aContext=0x0) at /home/cjones/mozilla/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:738
#7  0x00002baa0b42c18f in mozilla::net::HttpChannelChild::OnStartRequest (this=0x163c2a0, responseHead=..., useResponseHead=@0x7fff45f81680, requestHeaders=..., isFromCache=@0x7fff45f8167c, cacheEntryAvailable=@0x7fff45f81608, cacheExpirationTime=@0x7fff45f815c8, cachedCharset=..., securityInfoSerialization=...) at /home/cjones/mozilla/mozilla-central/netwerk/protocol/http/HttpChannelChild.cpp:262
#8  0x00002baa0b42bf39 in mozilla::net::HttpChannelChild::RecvOnStartRequest (this=0x163c2a0, responseHead=..., useResponseHead=@0x7fff45f81680, requestHeaders=..., isFromCache=@0x7fff45f8167c, cacheEntryAvailable=@0x7fff45f81608, cacheExpirationTime=@0x7fff45f815c8, cachedCharset=..., securityInfoSerialization=...) at /home/cjones/mozilla/mozilla-central/netwerk/protocol/http/HttpChannelChild.cpp:223
#9  0x00002baa0cabd41d in mozilla::net::PHttpChannelChild::OnMessageReceived (this=0x163c2a0, __msg=...) at PHttpChannelChild.cpp:435
#10 0x00002baa0ca3ec2b in mozilla::dom::PContentChild::OnMessageReceived (this=0x11fc1f0, __msg=...) at PContentChild.cpp:949

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't try to create plugin widgets in content processes, it's never going to work

With this bug fixed, the leak goes away.  I don't see any point in debugging code that's never going to be used, so here we are.

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 509921 [details] [diff] [review]
Don't try to create plugin widgets in content processes, it's never going to work

Questions: should this go higher up (in nsPluginInstanceOwner::CreateWidget)? Should we return an error instead of NS_OK?

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

You tell me! :)

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 509921 [details] [diff] [review]
Don't try to create plugin widgets in content processes, it's never going to work

So the patch is wrong because nsObjectFrame::CreateWidget doesn't always create a widget. If its called with aViewOnly (windowless plugins) then it doesn't create a widget. I'm not sure if we want to do the other stuff in this function if we are asking to create a widget in a content process.

I think nsObjectFrame::CreateWidget is the right place, and we should probably return an error code in this case.

Here is a question you should be able to answer: windowless plugins work in content processes?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #9)
> So the patch is wrong because nsObjectFrame::CreateWidget doesn't always create
> a widget. If its called with aViewOnly (windowless plugins) then it doesn't
> create a widget.

D'oh, thanks for the catch.

> I think nsObjectFrame::CreateWidget is the right place, and we should probably
> return an error code in this case.

OK.

> Here is a question you should be able to answer: windowless plugins work in
> content processes?

Yes, and were passing the modules/plugin reftests last time I checked.

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to comment #9)
> I'm not sure if we want to do the other stuff in this function
> if we are asking to create a widget in a content process.

I guess we should do everything else in CreateWidget except the widget specific stuff. (So skip the stuff inside "if (mWidget)" and "if (!aViewOnly && !mWidget)".)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't try to create plugin widgets in content processes, it's never going to work, v2

Fixes the leak.  This still causes some crashtests that were disabled for <browser remote> in bug 615386 to crash, probably because the test plugin is trying to use a null window ID.  Not worth bothering with yet, IMHO; I left the tests disabled.

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 510530 [details] [diff] [review]
Don't try to create plugin widgets in content processes, it's never going to work, v2

+    NS_WARNING("Can't use native widgets, and can't hand a plugins a PuppetWidget");

I thinks you need to fix the grammar.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops, been watching too much Chico Marx.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 510530 [details] [diff] [review]
Don't try to create plugin widgets in content processes, it's never going to work, v2

This patch prevents us from potentially doing bad things (creating unparented native widgets in content processes) and additionally has us pass more crashtests.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks.

http://hg.mozilla.org/mozilla-central/rev/02311190012b