658440 - null cx Crash [@ JS_updateMallocCounter] with Canvas

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://www.speechpathology.com/exams/preview/4187?print=ready
2. Mac|Linux|Windows Crash.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000038
0x066a9575 in JS_updateMallocCounter (cx=0x0, nbytes=1848) at /work/mozilla/builds/2.0.0/mozilla/js/src/jsapi.cpp:2020
2020	    return cx->runtime->updateMallocCounter(nbytes);
#1  0x052821b9 in nsCanvasRenderingContext2D::SetDimensions (this=0x1a171100, width=42, height=11) at /work/mozilla/builds/2.0.0/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:1128
#2  0x0533a374 in nsHTMLCanvasElement::UpdateContext (this=0x23b5f440, aNewContextOptions=0x0) at /work/mozilla/builds/2.0.0/mozilla/content/html/content/src/nsHTMLCanvasElement.cpp:609
#3  0x0533ac5c in nsHTMLCanvasElement::GetContext (this=0x23b5f440, aContextId=@0xbfffb4a4, aContextOptions=@0x6d8f078, aContext=0xbfffb49c) at /work/mozilla/builds/2.0.0/mozilla/content/html/content/src/nsHTMLCanvasElement.cpp:536
#4  0x05339bc8 in nsHTMLCanvasElement::CopyInnerTo (this=0x24fc4920, aDest=0x23b5f440) at /work/mozilla/builds/2.0.0/mozilla/content/html/content/src/nsHTMLCanvasElement.cpp:158
#5  0x0533b45d in nsHTMLCanvasElement::Clone (this=0x24fc4920, aNodeInfo=0x2506de90, aResult=0xbfffb5c0) at /work/mozilla/builds/2.0.0/mozilla/content/html/content/src/nsHTMLCanvasElement.cpp:101
#6  0x0520d400 in nsNodeUtils::CloneAndAdopt (aNode=0x24fc4920, aClone=1, aDeep=1, aNewNodeInfoManager=0x233aed50, aCx=0x0, aNewScope=0x0, aNodesWithProperties=@0xbfffc50c, aParent=0x23b75710, aResult=0xbfffb6e4) at /work/mozilla/builds/2.0.0/mozilla/content/base/src/nsNodeUtils.cpp:518

Comment 1

[Josh Matthews [:jdm]]

I'd be interested in seeing the full stack here.

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

bp-1b6c6027-b19f-4c17-8ca5-004212110520 (against http://hg.mozilla.org/mozilla-central/rev/2e0e36b0feae, WinXP).

Comment 3

[Josh Matthews [:jdm]]

Gal, the full stack shows that we're not running any javascript, so we can't get any sensible context here. Presuming that all we can do is null-check, is this something that should be handled in JS_updateMallocCounter or should callers be putting in the extra effort?

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: testcase

semi reduced testcase.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> Gal, the full stack shows that we're not running any javascript, so we can't
> get any sensible context here. Presuming that all we can do is null-check,
> is this something that should be handled in JS_updateMallocCounter or should
> callers be putting in the extra effort?

That function actually just uses the JSContext to get to the JSRuntime. I assume you can access the JSRuntime from the call site? We should probably just add another API for that.

Comment 6

[Bob Clary [:bc] (inactive)]

fyi, also on beta.

Comment 7

[Josh Matthews [:jdm]]

(In reply to comment #5)
> (In reply to comment #3)
> > Gal, the full stack shows that we're not running any javascript, so we can't
> > get any sensible context here. Presuming that all we can do is null-check,
> > is this something that should be handled in JS_updateMallocCounter or should
> > callers be putting in the extra effort?
> 
> That function actually just uses the JSContext to get to the JSRuntime. I
> assume you can access the JSRuntime from the call site? We should probably
> just add another API for that.

I'm just flailing around in the dark here: jst, would the correct way to get the runtime be something like this?

>nsCOMPtr<nsIJSRuntimeService> runtimeSvc = do_GetService("@mozilla.org/js/xpc/RuntimeService;1");
>JSRuntime* rt = nsnull;
>runtimeSvc->GetRuntime(&rt);

Comment 8

[Johnny Stenback (:jst)]

Yeah, that looks right.

Comment 9

[Josh Matthews [:jdm]]

Actually, I'm confused - is this still reproducible after bug 656947 landed? Specifically, is this reproducible in nightlies? If not, we could backport that patch and mark this bug as a DUP. Alternatively, if my current plan to obtain a runtime and add a new JS API call pans out, we could get more correct results than with the patch from the other bug.

Comment 10

[Bob Clary [:bc] (inactive)]

Only reproducible with this testcase on beta, not aurora or nightly. So bug 656947 probably did fix it. I'll leave it to you as to the best approach.

Comment 11

[Josh Matthews [:jdm]]

Attachment: Report memory usage more reliably.

I checked, and this patch is strictly better than the existing null-check as we can always obtain the runtime.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535558 [details] [diff] [review]
Report memory usage more reliably.

Looks ok to me.

Comment 13

[Andreas Gal :gal]

Comment on attachment 535558 [details] [diff] [review]
Report memory usage more reliably.

Isn't do_GetService pretty slow in that path? And also JS_updateMallocCounterRT is void but returns a value?

Comment 14

[Josh Matthews [:jdm]]

I guess we could try to get the context first, then fall back to the service.

Comment 15

[chris hofmann]

another one that looks like a definite volume regression in 5.0

         JS_updateMallocCounter
date     total    breakdown by build
         crashes  count build, count build, ...

20110520 7  	2 5.0a22011052004, 
        		1 6.0a12011052003, 	1 6.0a12011051803, 
        		1 5.0a22011051904, 	1 5.02011042714, 
        		1 4.0.12011041322, 
20110521 7  	6 5.02011051719, 
        		1 5.02011042714, 
20110522 10  	7 5.02011051719, 
        		2 5.0a22011052104, 	1 5.02011042714, 
20110523 14  	10 5.02011042714, 
        		3 5.02011051719, 	1 5.0a22011051904, 
20110524 23  	20 5.02011051719, 
        		2 5.0a22011052304, 	1 5.02011042714, 
20110525 29  	24 5.02011051719, 
        		3 5.02011042714, 	2 4.0.12011041322, 
20110526 40  	36 5.02011051719, 
        		2 5.0a22011052404, 	2 5.02011042714,

Comment 16

[Sheila Mooney]

Since this is a volume regression in 5.0 should we try and get approval for beta? Can someone add a risk assessment as well?

Comment 17

[Josh Matthews [:jdm]]

Attachment: Use the correct runtime to update the canvas memory counter more reliably.

Fixed up the void returns and added back the context path as well.

Comment 18

[Josh Matthews [:jdm]]

I don't believe there's any risk to this. We take a situation in which we used to crash and instead we now report better data about how much memory we're using.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 536148 [details] [diff] [review]
Use the correct runtime to update the canvas memory counter more reliably.

>+            if (runtimeSvc) {
>+                JSRuntime* rt = nsnull;
>+                (void) runtimeSvc->GetRuntime(&rt);
>+                if (rt) {
>+                    JS_updateMallocCounterRT(rt, width * height * 4);
>+                }
>+            }

So either this leaks, or that GetRuntime doesn't implement the contract it's supposed to. If it's the latter, please file a followup?

Comment 20

[Scoobidiver (away)]

Currently #13 top crasher in 5.0b3.

Comment 21

[Andreas Gal :gal]

Sorry for being so slow here. dmandelin seems to like adding the new API. I don't see the point of that. If there is no valid context around, no JS is running and keeping track of JS gcs doesn't make sense. I don't understand why the current if (context) bla code crashes. That solution seems sensible. Anyway, not worth dragging my feet over and this is crashing people. Lets just add the API.

Comment 22

[Josh Matthews [:jdm]]

Sorry gal, just to clear things up, this is no longer about a crash fix. That was taken care of by bug 656947. I simply assumed that (presumably) more accurate memory reporting was a better thing, which is why I've been pushing on this.

Comment 23

[Andreas Gal :gal]

In that case if there is no known issue with this, I would just leave it alone. We should work on a more generic memory pressure measurement system instead (i.e. measure working set size instead of counting mallocs).

Comment 24

[Josh Matthews [:jdm]]

Fine with me!

Comment 25

[Robert Kaiser]

(In reply to comment #20)
> Currently #13 top crasher in 5.0b3.

Actually, that's bug 656947, and is fixed for 5.0b4 :)