Closed Bug 349272 Opened 18 years ago Closed 18 years ago

Dealing with unwanted close hooks

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.8.1)

Attachments

(5 files, 7 obsolete files)

Something like a test case
220 bytes, text/html
Details
HTML to test the patch
1.01 KB, text/html
Details
Make the DOM code use the new API.
853 bytes, patch
mrbkap
: review+
brendan
: superreview+
Details | Diff | Splinter Review
Implementation v5
28.22 KB, patch
brendan
: review+
mrbkap
: review+
mtschrep
: approval1.8.1+
Details | Diff | Splinter Review
testcase modified form of attachment 238558
1.17 KB, text/html
Details 
Currently there is no way to control close hooks that GC schedules for execution to close generator objects. Thus a bad script can mount denial-of-service via a close hook with an infinite loop creating more generators with such close hook or even try to start XSS attack via a scenario given in bug 349012 comment 36.

Thus SM should provide API that an embedding can use to remove unwanted close hooks.
I think besides an API a useful default can also help to make JS embedding more robust.

For example, by default the engine can skip execution of GC-unreachable close hooks if the script that defined the generator became itself unreachable. In this way if the browser cleanups the page with a close handler, the handler would not be executed by default.  
In browser embeddings, <script> tags are compile-and-go, so the script for each tag's content is gone by the time the GC runs.  This would make all top-level use of close hooks futile.

Sure, Python can assume trust and not worry about DOS or other threats, but we should not worry excessively -- DOSes are a dime a dozen, in spite of our ongoing struggles to prevent them.  We really need to avoid worse threats, actual security bugs such as you've demonstrated.

I'm not against heuristics to reduce "close load", but they shouldn't break normal code that is not threatening.

/be
Blocks: geniter
(In reply to comment #2)
> In browser embeddings, <script> tags are compile-and-go, so the script for each
> tag's content is gone by the time the GC runs.  This would make all top-level
> use of close hooks futile.

Is it possible during GC purely by JS means to detect that the scope where the generator was defined was cleaned-up? If yes, then by default that can be used to cancel close hooks.
Igor, can you given an example of a "script that defined the generator" showing why it would be correct to skip closing the generator once the script is gone?

Would you be willing to take this bug?  It's blocking js1.7 and the release, so it needs an owner.  Thanks,

/be
This is a standalone version of bug 349012 comment 36.  Here the script copy for references:

<html><body><script>

function gen()
{
  function f()
  {
    prompt("Enter your password:");
  }
  try {
    yield 1;
  } finally {
    setTimeout(f, 5000);
  }
}

var iter = gen();
iter.next();
</script></body></html>

Depending on GC behavior, the script can prompt for password when a user returned  from the page with the script back to some other page in history.
Attachment #235541 - Attachment is patch: false
Attachment #235541 - Attachment mime type: text/plain → text/html
(In reply to comment #4)
> Would you be willing to take this bug?  It's blocking js1.7 and the release, so
> it needs an owner.  Thanks,

A question: where is the current scope cleanup code?
Assignee: general → igor.bukanov
This is important to fix for 1.8.1.  See the testcase.

/be
Flags: blocking1.8.1?
Flags: blocking1.8.1? → blocking1.8.1+
Attached patch Initial try (obsolete) — — Splinter Review 
This does not yet add any public API. Instead it makes sure that close hooks do not run if generator's parent scope is GC-unreachable. This should kill the close hooks when user closes the window. Now to postpone hooks for pages in history we need API and the question is which one?

One option is to require an embedding to define a hook which would tell if scope is hibernating. Another option is to have that an embedding can use to mark hibernated scopes. The advantage of the second option is that the mark can be read inside the locks while callbacks should be called outside them. But then I am not sure that a hidden property on a scope can always be set.
Attachment #238408 - Flags: review?
Attached patch Work in progress (obsolete) — — Splinter Review 
This version adds API to forcefully close all close hooks for the given scope. I extended JSTempValueRooter with a hook case since the code needed to root temporary list of close hooks from the given scope.

There is still no API to postpone the hooks.

Now I got a feeling that the end result would be simpler if instead of single global list of generators the code would use per-scope lists. It would really simplify all that searching code. But I am not sure that using hidden properties is a right way to implement this.
Attachment #238408 - Attachment is obsolete: true
Attachment #238414 - Flags: review?(brendan)
Attachment #238408 - Flags: review?
Comment on attachment 238414 [details] [diff] [review]
Work in progress

> /*
>+ * Execute all close hooks associated with the given scope including
>+ * GC-rechable onces.

"reachable"


>+/*
>+ * We do not run close hooks when the parent scope of the genartor instance

"generator instance"

>+ * becomes unrechable to prevent denial-of-service and resource leckage from

"unreachable", "leakage"

>+ * misbehaved generators.
>+ */
>+static JSBool
>+CanScheduleCloseHook(JSGenerator *gen)
>+{
>+    JSObject *parent;
>+
>+    /* Avoid OBJ_GET_PARENT overhead as we are in GC. */
>+    parent = JSVAL_TO_OBJECT(gen->obj->slots[JSSLOT_PARENT]);
>+    return *js_GetGCThingFlags(parent) & GCF_MARK;
>+}

If the parent is not marked, then the generator must be unreachable too.  Wouldn't it be faster to test whether gen->obj is marked?

>+
>+static JSBool
>+ShouldPostponeCloseHook(JSContext *cx, JSGenerator *gen)
> {
>-    for (; gen; gen = gen->next)
>-        GC_MARK(cx, gen->obj, "close list generator");
>+    return JS_FALSE;
> }

Is this future proofing worth it now?  I'm not against it in principle, just trying to minimize the patch for mozilla1.8.1.


>+            /* Discrard the generator from the list if its schedule is over. */

"Discard"


>-        ok = js_CloseGeneratorObject(cx, gen);
>-        if (cx->throwing) {
>-            /*
>-             * Report the exception thrown by the close hook and continue to
>-             * execute the rest of the hooks.
>-             */
>-            if (!js_ReportUncaughtException(cx))
>-                JS_ClearPendingException(cx);
>-            ok = JS_TRUE;
>-        } else if (!ok) {
>+        if (ShouldPostponeCloseHook(cx, gen)) {
>             /*
>-             * Assume this is a stop signal from the branch callback or other
>-             * quit ASAP condition. Break execution until the next invocation
>-             * of js_RunCloseHooks.
>+             * TODO: For now just reinsert the generator to the global list
>+             * as a simple thread-safe way to delay generator for at least the
>+             * following invocation of GC.
>              */
>-            break;
>+            js_RegisterGenerator(cx, gen);
>+        } else {
>+            ok = js_CloseGeneratorObject(cx, gen);
>+            if (cx->throwing) {
>+                /*
>+                 * Report the exception thrown by the close hook and continue
>+                 * to execute the rest of the hooks.
>+                 */
>+                if (!js_ReportUncaughtException(cx))
>+                    JS_ClearPendingException(cx);
>+                ok = JS_TRUE;
>+            } else if (!ok) {
>+                /*
>+                 * Assume this is a stop signal from the branch callback or
>+                 * other quit ASAP condition. Break execution until the next
>+                 * invocation of js_RunCloseHooks.
>+                 */
>+                break;
>+            }

Please provide a diff -w as well as the patch, if you don't un-future-proof to minimize patch.

> #endif /* JS_HAS_GENERATORS */
> 
> #if defined(DEBUG_brendan) || defined(DEBUG_timeless)
> #define DEBUG_gchist
> #endif
> 
> #ifdef DEBUG_gchist
>@@ -2620,15 +2768,15 @@ restart:
>     js_MarkAtomState(&rt->atomState, keepAtoms, gc_mark_atom_key_thing, cx);
>     js_MarkWatchPoints(rt);
>     js_MarkScriptFilenames(rt, keepAtoms);
>     js_MarkNativeIteratorStates(cx);
> 
> #if JS_HAS_GENERATORS
>     /* Mark generators whose close hooks were already scheduled to run. */
>-    MarkCloseList(cx, rt->gcCloseState.todoHead);
>+    MarkScheduledCloseHooks(cx);

Nit: MarkScheduledCloseables?  Just thinking "mark" verb does not apply to "hook" object, rather to "generator" or "closeable".  We avoided generalizing close phase, so MarkScheduledGenerators might be even better, because more concrete.

Looking pretty good -- I'll bring jst, he can most quickly get this called from the DOM code

/be
jst, we will need a quick patch today to call JS_ForceCloseHookExecution from DOM code when navigating away from a page.  The call should the inner window as the scope parameter.  Anything look wrong?

/be
Status: NEW → ASSIGNED
(In reply to comment #10)
> >+static JSBool
> >+CanScheduleCloseHook(JSGenerator *gen)
> >+{
> >+    JSObject *parent;
> >+
> >+    /* Avoid OBJ_GET_PARENT overhead as we are in GC. */
> >+    parent = JSVAL_TO_OBJECT(gen->obj->slots[JSSLOT_PARENT]);
> >+    return *js_GetGCThingFlags(parent) & GCF_MARK;
> >+}
> 
> If the parent is not marked, then the generator must be unreachable too. 
> Wouldn't it be faster to test whether gen->obj is marked?

??? Here is the generator is unreachable so by definition gen->obj is not marked. I need to check if the scope is unreachable as well to discard the close hook in that case. I do not see how checking for __parent__ can be avoided. 

> 
> >+
> >+static JSBool
> >+ShouldPostponeCloseHook(JSContext *cx, JSGenerator *gen)
> > {
> >-    for (; gen; gen = gen->next)
> >-        GC_MARK(cx, gen->obj, "close list generator");
> >+    return JS_FALSE;
> > }
> 
> Is this future proofing worth it now?  I'm not against it in principle, just
> trying to minimize the patch for mozilla1.8.1.

But this is just incomplete! We need API to tell the GC about hibernating scope and this is the place I want to put it. So I repeat the question:


Should the API be:
JS_SetHibernatingScope(cx, scope, true/false), 

or should it be a callback call to ask the embedding about hibernating? 

Without the API that test case would continue to work.

(In reply to comment #12)
> > If the parent is not marked, then the generator must be unreachable too. 
> > Wouldn't it be faster to test whether gen->obj is marked?
> 
> ???

!!! never mind, confusion before coffee this morning. Sorry about that!

> But this is just incomplete! We need API to tell the GC about hibernating scope
> and this is the place I want to put it. So I repeat the question:
> 
> Should the API be:
> JS_SetHibernatingScope(cx, scope, true/false), 
> 
> or should it be a callback call to ask the embedding about hibernating? 
> 
> Without the API that test case would continue to work.

We could force closing before leaving the page, so no hibernating and awakening. Then the only problem would be reachable generators being force-closed.  This would be a bug, for sure -- as if setTimeout with long delay, or setInterval, were cleared on leaving the page.

So to handle back/forward-cached pages with their live JSObjects, something like what you propose here is good.  How about preserving Close in the API naming and not mixing metaphors (as I've done in this bug): JS_DeferCloseHooks(cx, scope, flag)?  Can you implement this on top of the current patch?

/be

(In reply to comment #13)
> 
> So to handle back/forward-cached pages with their live JSObjects, something
> like what you propose here is good.  How about preserving Close in the API
> naming and not mixing metaphors (as I've done in this bug):
> JS_DeferCloseHooks(cx, scope, flag)?  Can you implement this on top of the
> current patch?

I will try to do that now via a hidden property in the scope, but if DOM already can tell if a scope is hibernating just from JSObject*, then it would be simpler to call a callback and ask about it.  

I said over IRC that the DOM can confirm or deny whether a scope object (inner window object) is the current one in session history, or in the bfcache, so the callback idea sounds best

jst, please correct this quickly if it's wrong.

/be
That's correct, nsGlobalWindow::IsFrozen() will tell you whether a window is in the bf cache or not.
jst, we were thinking of checking whether (sketching):

obj->outerObject()->innerObject() != obj

to see whether the (inner window) scope object obj is not current in history.  Is that sound?

/be
Attached patch Implementation v2 (obsolete) — — Splinter Review 
Note this is not yet tested in the browser. In addition I am not sure if outerObject/innerObject returning null indicate an error. The patch assumes it does.
Attachment #238414 - Attachment is obsolete: true
Attachment #238541 - Flags: review?(brendan)
Attachment #238414 - Flags: review?(brendan)
Attached patch Implementation v2b (obsolete) — — Splinter Review 
Same as above but without not yet written JS_DeferCloseHooks
Attachment #238541 - Attachment is obsolete: true
Attachment #238542 - Flags: review?(brendan)
Attachment #238541 - Flags: review?(brendan)
Comment on attachment 238542 [details] [diff] [review]
Implementation v2b

Testing is the thing.  Code looks good to me.  Asking jst for review.

/be
Attachment #238542 - Flags: review?(jst)
I'm having trouble testing this as in all my recent builds I get an error on the testcase

Error: missing ; before statement
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=235541
Line: 10, Column: 10
Source Code:
    yield 1;
----------^

I have one release build that's a trunk build from maybe a week ago or so, and there I can load that testcase successfully and I do see the problem, but in none of my debug builds am I able to get the testcase to run due to the reported error. And sicking is seeing the same thing too, so it's not just me.

Thoughts?
Attached file HTML to test the patch — 
This is a random test case that I use to test the implementation together with a debug printouts in jsgc.c.

A variation of the test reveals that if a generator creates a pending finally during document load and GC executed, then the generator is deferred as scope->outer->inner != scope. Is it expected during document load?
(In reply to comment #21)
> I'm having trouble testing this as in all my recent builds I get an error on
> the testcase
> 
> Error: missing ; before statement
> Source File: https://bugzilla.mozilla.org/attachment.cgi?id=235541
> Line: 10, Column: 10
> Source Code:
>     yield 1;

You need to change <script> into <script type="application/javascript;version=1.7">

as let and yield were made recently 1.7-only keywords. Or use the new test. In the new test you need go back after it prints out "nulling generator".

(In reply to comment #22)
> A variation of the test reveals that if a generator creates a pending finally
> during document load and GC executed, then the generator is deferred as
> scope->outer->inner != scope. Is it expected during document load? 

Ignore this: my printout was wrong ;)

Note that with the current patch if page creates many generators with pending finally and then user go back in history, all those generators would travel between reachable and scheduled lists back and forth on each GC execution. This is OK, but a picture like:

GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838
GEN: scheduling to run, gen=0x92474f8 parent=0x90d8838
GEN: deferring, gen=0x92474f8 parent=0x90d8838

that I got with going to the test case and then pressing back does not look nice.
Hey Guys - can this wait until 1.8.1.1 or is this critical for 1.8.1/FF2?  Worried about regression risk here and we are already pasted scheduled code freeze.
(In reply to comment #26)
> Hey Guys - can this wait until 1.8.1.1 or is this critical for 1.8.1/FF2? 
> Worried about regression risk here and we are already pasted scheduled code
> freeze.

Strictly my opinion: I would suggest to delay this until this until 1.8.1.1 and *patch* parser or interpreter to throw an internal error whenever yield happens inside try with finally. Then when we are 100% sure that generators 100% bulletproof against fishing examples like above we can enable the feature.
Attached patch Implementation v3 (obsolete) — — Splinter Review 
This version just keeps deferred close hooks on the scheduled list. For that it use JSTempCloseList to root all the hooks in js_RunCloseHooks which removed the need for todoCount. It also allows to stress test rooting through temp list in the shell.
Attachment #238570 - Flags: review?(brendan)
Attachment #238570 - Flags: review?(jst)
Attachment #238542 - Attachment is obsolete: true
Attachment #238542 - Flags: review?(jst)
Attachment #238542 - Flags: review?(brendan)
Comment on attachment 238570 [details] [diff] [review]
Implementation v3

- In ShouldDeferCloseHook():

+    parent = OBJ_GET_PARENT(cx, gen->obj);
+    clasp = OBJ_GET_CLASS(cx, parent);
+    if (clasp->flags & JSCLASS_IS_EXTENDED) {

Are we guaranteed that the parent of gen->obj is the global object here? If not you'd simply loop up the parent chain if necessary...

+            obj = xclasp->outerObject(cx, parent);
+            if (!obj) {
+#ifdef DEBUG_igor
+                fprintf(stderr, "GEN: Failed outerObject, gen=%p parent=%p\n",
+                        (void *)gen, (void *)parent);
+#endif
+                return JS_FALSE;
+            }
+            OBJ_TO_INNER_OBJECT(cx, obj);
+            if (!obj) {
+#ifdef DEBUG_igor
+                fprintf(stderr, "GEN: Failed innerObject, gen=%p parent=%p\n",
+                        (void *)gen, (void *)parent);
+#endif
+                return JS_FALSE;
+            }

I'm pretty sure null inners and outers can happen, but I can't seem to bring to memory what to do to trigger it. I'm guessing it's something that can happen during window destruction, but I don't remember for sure. I'm fairly sure that in neither case we'd want any code to run, so maybe just discard those close hooks and continue on with the rest?

r=jst with that dealt with.
Attachment #238570 - Flags: review?(jst) → review+
(In reply to comment #29)
> I'm pretty sure null inners and outers can happen, but I can't seem to bring to
> memory what to do to trigger it. I'm guessing it's something that can happen
> during window destruction, but I don't remember for sure.

The XPConnect innerObject and outerObject hooks will never return NULL without throwing an error. If we call innerObject or outerObject too early or too late (even in cases where we can deal successfully) you'll stop doing whatever you're doing (which is really annoying in some cases!).
This makes the DOM code use the new API in two cases. The first one being when we leave a page that will not be stuck in the fastback cache, the second one when a page falls out of the fastback cache.

The first case should be fine, but the second case is not ok. This callback then makes us call the close hook for the page that falls out of the fastback cache, and we do *not* want to do that, that page is long gone as far as the user is concerned.

Should we simply not call the new hook in the second case? Would that let the close hooks get collected w/o being called? If so, we only need the second part of this diff.
Attachment #238583 - Flags: superreview?(brendan)
Attachment #238583 - Flags: review?(igor.bukanov)
(In reply to comment #29)
> (From update of attachment 238570 [details] [diff] [review] [edit])
> - In ShouldDeferCloseHook():
> 
> +    parent = OBJ_GET_PARENT(cx, gen->obj);
> +    clasp = OBJ_GET_CLASS(cx, parent);
> +    if (clasp->flags & JSCLASS_IS_EXTENDED) {
> 
> Are we guaranteed that the parent of gen->obj is the global object here?

Yes.  See jsobj.c, js_NewObject, in the case where a non-null proto for the new object has been passed in or found via either indexing into registered standard prototypes or searching globally-defined class constructors:

        /*
         * Default parent to the parent of the prototype, which was set from
         * the parent of the prototype's constructor.
         */
        if (!parent)
            parent = OBJ_GET_PARENT(cx, proto);

The Generator class is anonymous.  The Generator class prototype is parented to the global object with which JS_InitClass was called (as the second param, obj) by the above code, where proto is Object.prototype.  Object.prototype, as any class prototype, is created with an explicit parent of the global object (the obj param again), in jsapi.c, JS_InitClass:

    /* Create a prototype object for this class. */
    proto = js_NewObject(cx, clasp, parent_proto, obj);
    if (!proto)
        return NULL;

/be
Comment on attachment 238570 [details] [diff] [review]
Implementation v3


> /*
>+ * Execute all close hooks associated with the given scope including
>+ * GC-reachable ones.
>+ */
>+extern JS_PUBLIC_API(JSBool)
>+JS_ForceCloseHookExecution(JSContext *cx, JSObject *scope);

Comment might say "generator close hooks ...".

>+/*
>+ * Check if we should delay execution of the close hook.
>+ *
>+ * Called outside GC or any locks.
>+ *
>+ * XXX The current implementation is a hack that embedds the knowledge of the

"embeds"

>+ * browser embedding pending the resolution of bug ???. In the browser we must

Cite this bug, or a followup bug on file?

Looks great, r=me with these comment tweaks.

/be
Attachment #238570 - Flags: review?(brendan) → review+
Attachment #238583 - Flags: superreview?(brendan)
Attachment #238583 - Flags: superreview+
Attachment #238583 - Flags: review?(mrbkap)
Blocks: 352788
Attached patch Implementation v3b (obsolete) — — Splinter Review 
The patch to commit with comment fixes.

Note that I have an extra patch on top of it to eliminate JSGCCloseState.todoTail making the code smaller. But since this a pure optimization that would need an extra review so I will deal with it separately.
Attachment #238570 - Attachment is obsolete: true
(In reply to comment #31)
> Created an attachment (id=238583) [edit]
> Make the DOM code use the new API.
> 
> This makes the DOM code use the new API in two cases. The first one being when
> we leave a page that will not be stuck in the fastback cache, the second one
> when a page falls out of the fastback cache.
> 
> The first case should be fine, but the second case is not ok. This callback
> then makes us call the close hook for the page that falls out of the fastback
> cache, and we do *not* want to do that, that page is long gone as far as the
> user is concerned.

Then the second case would introduce exactly the problem this bug is supposed to address: the code should only be executed when the page is visible or an alert from the history can be triggered on a banking site etc.

I think the best would be to to change JS_ForceCloseHookExecution to execute only GC-unreachable generators and then the browser should always execute JS_CloseUnreachableGenerators when the page would go the history and that is it.


Comment on attachment 238599 [details] [diff] [review]
Implementation v3b

>+    if (tempList.head) {
>+        /*
>+         * Some close hooks were not yet executed, put them back into the
>+         * scheduled list.
>+         */
>+        while ((gen = *genp) != NULL) {
>+            genp = &gen->next;
>+            METER(deferCount++);
>+        }
>+
>+        /* Now genp is a pointer to the tail of tempList. */
>+        JS_LOCK_GC(rt);
>+        if (!rt->gcCloseState.todoHead)
>+            rt->gcCloseState.todoTail = genp;
>+        rt->gcCloseState.todoHead = tempList.head;

This is a bug: I missed assignment
    *genp = rt->gcCloseState.todoHead;

Late night hacking, you know.
Attachment #238599 - Attachment is obsolete: true
Attached patch Implementation v4 (obsolete) — — Splinter Review 
Fixing bugs, eliminating todoTail and better commenting.
Attachment #238607 - Flags: review?(brendan)
Attachment #238607 - Flags: superreview?(mrbkap)
Attachment #238607 - Flags: review?(jst)
With the latest patch my second paragraph in comment #31 still applies. How do we prevent that? 
Comment on attachment 238607 [details] [diff] [review]
Implementation v4

> /*
>+ * Close all closable objects such as generators associated with the given

"closeable"

>+ * scope or all closable object if the scope is null. The method closes both
>+ * GC-reachable and unreachable objects.
>+ */
>+extern JS_PUBLIC_API(JSBool)
>+JS_ForceCloseHookExecution(JSContext *cx, JSObject *scope);

Suggest s/scope/global/g or s/scope/parent/g to avoid confusion with JSScope *scope elsewhere in the engine (but not in the API, so this is just a suggestion -- still, parent would be precise in a reductive sense, and global would be crisper since this is a global scope object, not just any scope object).


>             /*
>-             * Only generators that yielded or were already closed can become
>-             * unreachable while still on reachableList.
>+             * Generator must not be executing when it becomes unrechable.

"unreachable"


>         /*
>          * Mark just found unreachable generators *after* we scan the global

s/just found/just-found/

>          * list so a generator that refers to other unreachable generators
>          * cannot keep them on gcCloseState.reachableList.

"to prevent a generator that refers to other unreachable generators from keeping them on ...".

>          */
>-        MarkCloseList(cx, todo);
>+        for (gen = todo; gen; gen = gen->next)
>+            GC_MARK(cx, gen->obj, "newly scheduled generator");
>     }
> }
> 
>+/*
>+ * Mark unrechable generators already scheduled to close and return the tail

"unreachable"

>+    /* First collect alreday scheduled hooks. */

"already", and want "already-scheduled".

>+    /* Now deal with so far GC-reachable hooks. */

s/so far/known/

r=me, making mrbkap additional review.

/be
Attachment #238607 - Flags: superreview?(mrbkap)
Attachment #238607 - Flags: review?(mrbkap)
Attachment #238607 - Flags: review?(brendan)
Attachment #238607 - Flags: review+
(In reply to comment #38)
> With the latest patch my second paragraph in comment #31 still applies. How do
> we prevent that? 

We don't want to keep deferring after this point, we simply want to forget about these generators.  A second API, CloseUnreachableGenerators as Igor said in comment 35, is necessary.

/be
(In reply to comment #40)
> (In reply to comment #38)
> > With the latest patch my second paragraph in comment #31 still applies. How do
> > we prevent that? 
> 
> We don't want to keep deferring after this point, we simply want to forget
> about these generators.  A second API, CloseUnreachableGenerators as Igor said
> in comment 35, is necessary.

After playing with the patch my conclusion is that there is no point to bother with public API since the patch should work as is. With the patch close hooks never run for hibernating pages and were quickly scheduled whenever the page that created generators was brough back from history. Thus the patch alone is sufficient to address the original problem. 

Thus I will make a new version of the patch to reflect the above suggestions but it will not contain any traces of js_ForceCloseHookExcecution. The API should go to 352788.
This is effectively v4 plus nits plus extra asserts munus JS_ForceCloseHookExecution to have smaller patch that works.
Attachment #238607 - Attachment is obsolete: true
Attachment #238681 - Flags: superreview?(mrbkap)
Attachment #238681 - Flags: review?(brendan)
Attachment #238607 - Flags: review?(mrbkap)
Attachment #238607 - Flags: review?(jst)
Attachment #238681 - Flags: review?(jst)
Comment on attachment 238681 [details] [diff] [review]
Implementation v5

Of course!  When the inner window can be GC'ed (is not marked after mark phase), then we won't defer even if it does not lead via outerObject then innerObject back to itself!

This was the readon I mentioned the outer/innerObject hooks on IRC as relieving us from needing new callbacks (but then lamented the tight coupling to the DOM embedding for which we have a followup bug).  Too much going on, and it seemed like comment in the bug showed a problem without explicit API.  I should have clung harder to the original inner->outer->other-inner insight.

Nice to see a reduced patch; thanks for following the train of thought through to the end of the line!

/be
Attachment #238681 - Flags: review?(brendan) → review+
Comment on attachment 238681 [details] [diff] [review]
Implementation v5

r=jst, seems to work as expected in the cases where I've seen undesired behavior with the earlier patches.
Comment on attachment 238681 [details] [diff] [review]
Implementation v5

Blake certainly should review when he has time, but this patch should land ASAP on the trunk, and be nominated for the 1.8 branch today.

/be
Attachment #238681 - Flags: superreview?(mrbkap) → review?(mrbkap)
(In reply to comment #45)
> (From update of attachment 238681 [details] [diff] [review] [edit])
> Blake certainly should review when he has time, but this patch should land ASAP
> on the trunk, and be nominated for the 1.8 branch today.

To Brendan:

The tinderboxes are read and it is too late here to commit in any case. So could you land the patch?

Sure, I'll land the "Implementation v5" patch.

/be
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.118; previous revision: 3.117
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.176; previous revision: 3.175
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.57; previous revision: 3.56
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.40; previous revision: 3.39
done

/be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 238681 [details] [diff] [review]
Implementation v5

This is a js/src change only, so jst's review is welcome but not required (testing feedback is good too).  Ditto mrbkap.  Asking for 1.8.1 late approval to get the full js1.7 generator close protocol browser-safe.

/be
Attachment #238681 - Flags: approval1.8.1?
Comment on attachment 238681 [details] [diff] [review]
Implementation v5

a=schrep so we can get in the nightlies.
Attachment #238681 - Flags: approval1.8.1? → approval1.8.1+
I committed the patch from comment 42 to MOZILLA_1_8_BRANCH:

Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.13; previous revision: 3.80.4.12
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.22; previous revision: 3.104.2.21
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.33.4.6; previous revision: 3.33.4.5
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.16; previous revision: 3.17.2.15
done
Keywords: fixed1.8.1
I've added a test based on Igor's to an internal test suite.
Flags: in-testsuite+
Attachment #238583 - Flags: review?(mrbkap) → review+
Attachment #238681 - Flags: review?(mrbkap) → review+
The testcase I made from attachment 235541 [details]: Something like a test case did not appear to show this bug, but when modifying attachment  238558 [details]: HTML to test the patch to load gmail, it can show the prompt for the password when the gmail page is displayed.

This may be this specific bug or it may be a different one.
(In reply to comment #53)
> This may be this specific bug or it may be a different one. 

This is related to bug 52209: the browser really should not execute any scripted code while there are modal dialog. 


(In another reply to comment #53)
...
> to load gmail, it can show the prompt for the password when the gmail
> page is displayed.

I tried the same test with just

    setTimeout("document.location.href = 'http://gmail.com/'", 2000);
    prompt("Enter your password 1:");

but then gmail was never loaded fully. It seems that final blocks from generator made that to work for some reason. I think we need another bug on this.
(In reply to comment #55)
> I think we need another bug on this.

could you file it please as I am betting your bug would be better than the one I would file.
Attachment #238583 - Flags: review?(igor.bukanov)
Attachment #238681 - Flags: review?(jst)
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: