Closed Bug 773980 Opened 12 years ago Closed 12 years ago

Fix browser-api randomorange by not touching dead windows

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox15 + wontfix
firefox16 + wontfix

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(2 files, 1 obsolete file)

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?
Blocks: hueyfix
Blocks: 765242
Blocks: 774235
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
Attached patch Part 1: Add Cu.isDeadWrapper. (obsolete) — Splinter Review
Attachment #643358 - Flags: review?(bobbyholley+bmo)
> +    dump("XXX _sendDOMRequest(" + msgName + ")\n");

Oops.  Removed locally.
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. ;-)
Attachment #643358 - Flags: review?(bobbyholley+bmo) → review-
>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...
> Would it help if you thought of this as a test of bug 769273

er, bug 695480.
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!
(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?
> 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.
Attachment #643359 - Flags: review?(mounir) → review+
I may have messed up the namespacing here; let me know if I need to rework things again.
Attachment #643875 - Flags: review?(bobbyholley+bmo)
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. ;-)
Attachment #643875 - Flags: review?(bobbyholley+bmo)
Attachment #643875 - Flags: review+
Attachment #643358 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2ab27745f1dd
https://hg.mozilla.org/mozilla-central/rev/3a20b26a4907
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 776116
We need part 1 of this bug on beta so Greasemonkey can fix bug 778318.  Justin, can you backport part 1?
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.
Attachment #643875 - Flags: approval-mozilla-beta?
Attachment #643875 - Flags: approval-mozilla-aurora?
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.
Attachment #643875 - Flags: approval-mozilla-beta?
Attachment #643875 - Flags: approval-mozilla-beta+
Attachment #643875 - Flags: approval-mozilla-aurora?
Attachment #643875 - Flags: approval-mozilla-aurora+
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.
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.
Blocks: 780546
No longer blocks: 780546
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.