Last Comment Bug 773980 - Fix browser-api randomorange by not touching dead windows
: Fix browser-api randomorange by not touching dead windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 776116
Blocks: hueyfix 765242 774235 778318
  Show dependency treegraph
 
Reported: 2012-07-14 11:24 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-04-04 13:53 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
wontfix


Attachments
Part 1: Add Cu.isDeadWrapper. (7.05 KB, patch)
2012-07-18 06:59 PDT, Justin Lebar (not reading bugmail)
bobbyholley: review-
Details | Diff | Review
Part 2: Don't touch dead objects in BrowserElementParent.js. (4.05 KB, patch)
2012-07-18 06:59 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Review
Part 1, v2: Add Cu.isDeadWrapper (13.26 KB, patch)
2012-07-19 08:59 PDT, Justin Lebar (not reading bugmail)
bobbyholley: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-07-14 11:24:10 PDT
I've been investigating browser-api randomorange, and I think it's mostly due to the code touching dead window objects.  We do some stuff and finish the test, but the docshell lives on and sends BrowserElementParent.js a few extra notifications, after the corresponding window is dead.

This is a relatively simple thing to fix.  The only tricky point is, how do we avoid having each BrowserElementParent register an inner-window-destroyed notification?  As dbaron points out in bug 770993 comment 18, this could lead to a lot more notifications than we want.

The obvious solution would be to have the BrowserElementParentFactory listen for inner-window-destroyed.  Then we'd only have one listener instead of one for each BrowserElementParent.

But just to throw it out there: This pattern appears elsewhere (e.g. bug 770993), and instead of requiring everyone to make their own singleton observing inner-window-destroyed, it would be a lot simpler if I could cleanly query an object to discover if it's dead (as opposed to touching the object from a try/catch).  Looking at the DeadObjectProxy code, that doesn't even seem too hard.  What would you think of that approach, Kyle?
Comment 1 Justin Lebar (not reading bugmail) 2012-07-14 12:14:12 PDT
https://tbpl.mozilla.org/?tree=Try&rev=57b21ce978f6
Comment 2 Justin Lebar (not reading bugmail) 2012-07-16 08:24:48 PDT
https://tbpl.mozilla.org/?tree=Try&rev=68f1ab60fc70
Comment 3 Justin Lebar (not reading bugmail) 2012-07-16 10:27:00 PDT
One of these is going to work.  https://tbpl.mozilla.org/?tree=Try&rev=510b278adbc2
Comment 4 Justin Lebar (not reading bugmail) 2012-07-17 15:00:59 PDT
https://tbpl.mozilla.org/?tree=Try&rev=2b21aaa29170
Comment 5 Mozilla RelEng Bot 2012-07-17 22:45:24 PDT
Try run for 2b21aaa29170 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2b21aaa29170
Results (out of 49 total builds):
    success: 43
    warnings: 5
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-2b21aaa29170
Comment 6 Justin Lebar (not reading bugmail) 2012-07-18 06:59:31 PDT
Created attachment 643358 [details] [diff] [review]
Part 1: Add Cu.isDeadWrapper.
Comment 7 Justin Lebar (not reading bugmail) 2012-07-18 06:59:55 PDT
Created attachment 643359 [details] [diff] [review]
Part 2: Don't touch dead objects in BrowserElementParent.js.
Comment 8 Justin Lebar (not reading bugmail) 2012-07-18 08:02:53 PDT
> +    dump("XXX _sendDOMRequest(" + msgName + ")\n");

Oops.  Removed locally.
Comment 9 Bobby Holley (busy) 2012-07-18 13:28:06 PDT
Comment on attachment 643358 [details] [diff] [review]
Part 1: Add Cu.isDeadWrapper.


> 
>+    virtual bool isDeadWrapper() {
>+        return false;
>+    }
>+

The virtual method here isn't appropriate (isOuterWindow is an old hack that needs to go away). Just add js::IsDeadWrapper to jsfriendapi, and preferable have it just take a JSObject* rather than making consumers grab the proxy handler.


>+    /**
>+     * Determines whether this object is backed by a DeadObjectProxy.
>+     */
>+    bool isDeadWrapper(in jsval obj);
>+

A little more explanation here would be useful.

>+NS_IMETHODIMP
>+nsXPCComponents_Utils::IsDeadWrapper(const jsval &obj, bool *out)
>+{
>+    *out = false;
>+
>+    if (JSVAL_IS_PRIMITIVE(obj))
>+        return NS_OK;
>+
>+    JSObject *object = js::UnwrapObject(JSVAL_TO_OBJECT(obj));

Why the unwrap call here? Sprinkling unwraps around is generally bad.

>diff --git a/js/xpconnect/tests/browser/browser_dead_object.js b/js/xpconnect/tests/browser/browser_dead_object.js

Could you instead do a simple xpcshell test and trigger the nuke with the machinery from bug 769273, which just landed? The way we nuke wrappers on window destruction is an implementation heuristic IMO, so I'd rather not codify it here. Also, I never remember to run browser-chrome tests locally. ;-)
Comment 10 Justin Lebar (not reading bugmail) 2012-07-18 13:31:02 PDT
>diff --git a/js/xpconnect/tests/browser/browser_dead_object.js b/js/xpconnect/tests/browser/browser_dead_object.js

Would it help if you thought of this as a test of bug 769273, which incidentally tests Cu.isDeadWrapper?  At the moment, we have no tests for that bug...
Comment 11 Justin Lebar (not reading bugmail) 2012-07-18 13:34:04 PDT
> Would it help if you thought of this as a test of bug 769273

er, bug 695480.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-07-18 13:35:45 PDT
Bug 695480 may not have direct tests, but it does have a lot of indirect ones (e.g. the leaked windows count).

Not terribly relevant, but I wanted to point out that I didn't land without any tests!
Comment 13 Bobby Holley (busy) 2012-07-18 13:36:09 PDT
(In reply to Justin Lebar [:jlebar] from comment #11)
> > Would it help if you thought of this as a test of bug 769273
> 
> er, bug 695480.

Yes, that's reasonable. Can you also add a quick check for this in test_nuke_sandbox.js as well though?
Comment 14 Justin Lebar (not reading bugmail) 2012-07-18 13:38:23 PDT
> Not terribly relevant, but I wanted to point out that I didn't land without any tests!

Fair enough!  :)

> Can you also add a quick check for this in test_nuke_sandbox.js as well though?

Definitely.
Comment 15 Justin Lebar (not reading bugmail) 2012-07-19 08:59:50 PDT
Created attachment 643875 [details] [diff] [review]
Part 1, v2: Add Cu.isDeadWrapper

I may have messed up the namespacing here; let me know if I need to rework things again.
Comment 16 Bobby Holley (busy) 2012-07-19 09:27:17 PDT
Comment on attachment 643875 [details] [diff] [review]
Part 1, v2: Add Cu.isDeadWrapper


>+NS_IMETHODIMP
>+nsXPCComponents_Utils::IsDeadWrapper(const jsval &obj, bool *out)
>+{
>+    if (JSVAL_IS_PRIMITIVE(obj))
>+        return NS_OK;

You need to set *out here if you're returning NS_OK, otherwise it's left undefined. I'm also of the opinion that we should throw in that case, because there's no good boolean answer to give if we're not passed an object.

r=bholley with that addressed. no double r+ though. ;-)
Comment 17 Justin Lebar (not reading bugmail) 2012-07-19 09:41:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab27745f1dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a20b26a4907

Thanks for the quick reviews, Mounir and Bobby!
Comment 19 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-07-31 18:30:18 PDT
We need part 1 of this bug on beta so Greasemonkey can fix bug 778318.  Justin, can you backport part 1?
Comment 20 Justin Lebar (not reading bugmail) 2012-07-31 18:59:45 PDT
Comment on attachment 643875 [details] [diff] [review]
Part 1, v2: Add Cu.isDeadWrapper

[Approval Request Comment]
This is needed to fix bug 778318, a major leak in Greasemonkey.  The bad behavior in Greasemonkey was caused by bug 695480; as such, we need this on Aurora and Beta (FF15 and newer).

This patch is low-risk because this patch does not modify any existing behavior; it only adds a new function.  In FF15 and FF16, no code in Firefox proper will invoke the function added by this patch.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-01 15:32:34 PDT
Comment on attachment 643875 [details] [diff] [review]
Part 1, v2: Add Cu.isDeadWrapper

Since this is low risk and doesn't change behaviour we can take this for beta 4, approving for branches.
Comment 22 Justin Lebar (not reading bugmail) 2012-08-02 07:48:57 PDT
Part 1 (Cu.isDeadWrapper) landed in Beta and Aurora for FF15, FF16:

https://hg.mozilla.org/releases/mozilla-beta/rev/e192796351bc
https://hg.mozilla.org/releases/mozilla-aurora/rev/baf781443c5d

I'm not marking this bug as fixed in FF15/16 because I didn't land part 2, which fixes the problem in this bug's summary.
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-02 15:07:51 PDT
Thanks Justin, I'm going to mark them as wontfix with regards to the actual bug summary then knowing that the bug comment history explains clearly what happened so that they stay out of our triage queues now that there's nothing left to do here.

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