Closed Bug 631585 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
One leak is from content/media/test/crashtests/459439-1.html.
... by which I meant HTTP load 481136-1.html.  (Messed up the bisect.)
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).
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?
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
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.
Assignee: nobody → jones.chris.g
Attachment #509921 - Flags: review?(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 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?
(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.
(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)".)
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.
Attachment #509921 - Attachment is obsolete: true
Attachment #510530 - Flags: review?(tnikkel)
Attachment #509921 - Flags: review?(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.
Attachment #510530 - Flags: review?(tnikkel) → review+
Oops, been watching too much Chico Marx.
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.
Attachment #510530 - Flags: approval2.0?
Attachment #510530 - Flags: approval2.0? → approval2.0+
Thanks.

http://hg.mozilla.org/mozilla-central/rev/02311190012b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.