352264 - audit code for asymmetric push/pop of JS context stack

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are a number of topcrashes for which the only reasonable explanation brendan and I could come up with was that the context at the top of the JS context stack has been deleted.  This could happen if a caller forgot to pop a context that was pushed.

The crashes that I'm looking at are:
bug 317283
bug 319533
bug 322056 comment 35
bug 322143
bug 325547
and they seem to be Windows-only.

Most of my audit for asymmetric (either way) pushes and pops consisted of looking through http://lxr.mozilla.org/seamonkey/search?string=ContextStack%3B1 , although I started from the XPConnect code that maintains the stack and worked my way out.  The problems or potential problems that I found are the following (roughly from most alarming to least alarming, although I didn't sort very carefully):

modules/plugin/base/src/nsJSNPRuntime.cpp
  AutoCXPusher doesn't hold on to sContextStack -- does it need to?
  (could sWrapperCount drop to zero or increase from zero in the middle?)
  (if not, why are there null checks?)

dom/src/base/nsJSEnvironment.cpp#1678
  has an NS_ENSURE_SUCCESS
  checking result of ConvertSupportsTojsvals

js/src/xpconnect/src/xpccallcontext.cpp
  XPCCallContext's constructor does its Push before ensuring that
  mXPCContext is non-null, which means it might not pop.  (Maybe safer
  to just fix the destructor, though.)

modules/plugin/base/src/ns4xPlugin.cpp
  That we expose Push/Pop through the plugin API
  (NPPVjavascriptPushCallerBool) scares me a good bit.

  if GetPeer succeeds but returns null, the error in Push is not
  propagated to the caller

  We should probably add code to assert, and perhaps even enforce, that
  plugin push/pops are symmetric and match the C call stack.  We should
  at least assert that plugin pops are popping the context they pushed.

embedding/browser/activex/src/plugin/LegacyPlugin.cpp
js/src/liveconnect/nsCLiveconnect.cpp#113
  doesn't null out mContextStack if Peek fails

embedding/components/windowwatcher/src/nsWindowWatcher.cpp
xpfe/bootstrap/nsNativeAppSupportOS2.cpp#2274
xpfe/bootstrap/nsNativeAppSupportWin.cpp#2288
toolkit/xre/nsNativeAppSupportWin.cpp#1486
toolkit/xre/nsNativeAppSupportOS2.cpp#1630
  assumes mService->GetSafeJSContext(&mContext) doesn't simultaneously
  fail and set mContext

The implementation of Push needs to fail on OOM, and then callers (most
of them!) need to check the result.

Many places also use Pop(&cx) when they could use Pop(nsnull).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for some of the issues

This fixes some of the issues.  Apologies about the readability of the diff, but I'm not sure what I could do to improve it.

It notably doesn't fix the first; I want to discuss that one with jst.

Also note that for the two files where the issue was incorrect error handling of the result of Peek, I intentionally reversed the error handling for Peek failures so that we do try to Push (and check the result of Push too).

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for some of the issues

I should really read the diff before attaching it...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237901 [details] [diff] [review]
patch for some of the issues

sr=bzbarsky

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for nsJSNPRuntime

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 237901 [details] [diff] [review]
patch for some of the issues

r=jst

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 238081 [details] [diff] [review]
patch for nsJSNPRuntime

r+sr=jst

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Above two patches checked in to the trunk.

Comment 8

[Mike Schroepfer]

Comment on attachment 237901 [details] [diff] [review]
patch for some of the issues

a=schrep for 181drivers for topcrash prevention.

Comment 9

[Mike Schroepfer]

Comment on attachment 238081 [details] [diff] [review]
patch for nsJSNPRuntime

a=schrep for 181drivers for topcrash prevention.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_8_BRANCH.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If we have a set of security testcases that are easy to run, testing stuff related to the JS context stack (i.e., whether the topmost JS is chrome or content), it might be worth running them to make sure I didn't mess anything up here...

Comment 12

[Bob Clary [:bc] (inactive)]

(In reply to comment #11)

I have *some* regression tests that were mined from bugzilla and recently did a run for 1.8 and can do another as soon as I catch up with js tests. They aren't automated unfortunately and take several hours per platform. I'll try to have them complete by tomorrow sometime.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm.  This doesn't seem to have fixed the crashes, or at least not all of them: TB23292220 came in today.

Comment 14

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 237901 [details] [diff] [review]
patch for some of the issues

approved for 1.8.0 branch, a=dveditz for drivers

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_8_0_BRANCH.

Comment 17

[Jay Patel [:jay]]

Adding [need testcase] since there is no easy way for QA to verify this fix.  We will continue to keep an eye on Talkback data.

bc:  Anything to report from the testing you did back in comment #10?  I'm assuming everything was fine from the lack of comments in this bug since the checkin on Trunk and 1.8 branch.

Comment 18

[Bob Clary [:bc] (inactive)]

(In reply to comment #17)

> bc:  Anything to report from the testing you did back in comment #10?  I'm
> assuming everything was fine from the lack of comments in this bug since the
> checkin on Trunk and 1.8 branch.

I ran the security regression tests for firefox around rc1, but I'm not sure that it was an effective test of this.

Comment 19

[Nochum Sossonko [:Natch]]

Can this be closed?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should probably have something like a static analysis for this...

Comment 21

[Negritas Sergiu, Desktop QA]

Hello David! Is this issue still available or we can close it?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

The relevant code has changed quite a bit with AutoJSAPI and such. I don't think this bug is particularly useful anymore.