Last Comment Bug 659215 - Try harder to get a JSContext in PostMessageEvent::Run
: Try harder to get a JSContext in PostMessageEvent::Run
Status: RESOLVED FIXED
[qa-]
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks: 553125
  Show dependency treegraph
 
Reported: 2011-05-23 20:05 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-09-22 13:50 PDT (History)
3 users (show)
khuey: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
Patch (3.91 KB, patch)
2011-05-24 10:20 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review+
Details | Diff | Splinter Review
Cleaner patch (1.37 KB, patch)
2011-05-24 13:04 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
khuey: review+
jst: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-23 20:05:53 PDT
The patch from Bug 553125 causes a leak and an assertion if we can't get a JSContext from the window.  It turns out that that can be hit if one calls postMessage on a window that's closed.  Our testsuite actually does this.

We should try harder to get a JSContext to use.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-24 10:20:59 PDT
Created attachment 534822 [details] [diff] [review]
Patch
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-05-24 10:57:40 PDT
Comment on attachment 534822 [details] [diff] [review]
Patch

Review of attachment 534822 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +5903,5 @@
> +    if (cxStack) {
> +      rv = cxStack->GetSafeJSContext(&cx);
> +    }
> +
> +    if (!cxStack || NS_FAILED(rv) || !cx) {

I think you can get away with just checking !cx here, and don't worry about getting a return value from GetSafeJSContext.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-24 12:07:45 PDT
I fixed that, and reverted the now unnecessary nsresult decl changes.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-24 13:04:14 PDT
Created attachment 534868 [details] [diff] [review]
Cleaner patch

Carrying forward r=bent.

We should take this on Aurora to fix a minor memory leak.  Risk here should be low.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-24 14:03:58 PDT
http://hg.mozilla.org/mozilla-central/rev/a2b3d055d1f8

Sadly we can't test this since assertions are not fatal in any test suite that can use window.open.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-03 09:39:33 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/3cb769e43035
Comment 7 Trif Andrei-Alin[:AlinT] 2011-08-19 04:44:16 PDT
so is it safe to say that this issue was fixed?
Thanks.
Comment 8 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 13:50:44 PDT
qa-: nothing for QA to verify

Note You need to log in before you can comment on or make changes to this bug.