Prevent Geolocation making callbacks to a ScriptExecutionContext that no longer exists

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Steve Block]

Geolocation callbacks are invoked using the ScriptExecutionContext used to make the corresponding method call. In the case where that context is not the context of the frame that owns the Geolocation object, we can't rely on disconnectFrame() to tell us when that context has gone away. Instead, we must check at callback time.

[Steve Block]

Created attachment 57865 [details]
Patch

[Steve Block]

The fix for this proposed in https://bugs.webkit.org/show_bug.cgi?id=39879#c11 was have the callbacks inherit from ActiveDOMObject, so as to provide way to know when the ScriptExecutionContext has gone away.

The problem is that the ScriptExecutionContext is ref'ed from script, so isn't destroyed until the GC runs. So we may not get the notification that the context is gone until some time after it is disconnected from its frame. But the current code path in JSCallbackData obtains the ScriptExecutionContext through DOMWindow::document() which in turn uses the Frame. So there exists a time between frame detachment and garbage collection when JSCallbackData can't get to the ScriptExecutionContext, but we haven't yet been notified through ActiveDOMObject that it's gone.

The call to DOMWindow::document() fails gracefully if the frame is detached, so we can fix this bug by modifying JSCallbackData without using ActiveDOMObject.

If we want to use ActiveDOMObject, I think more substantial changes will be required.

[Jeremy Orlow]

Comment on attachment 57865 [details]
Patch

LayoutTests/ChangeLog:8
 +          * fast/dom/Geolocation/callback-to-deleted-context-expected.txt: Added.
This is the only one that showed up in the diff.

WebCore/ChangeLog:6
 +          https://bugs.webkit.org/show_bug.cgi?id=40162
All the detail in the bug should be here as well.

I'm fine with doing this change then looking at doing ActiveDOMObject, but I still think the latter should be done.

[Jeremy Orlow]

Comment on attachment 57865 [details]
Patch

nm...see the files now..not sure what happened

As discussed, I think this is a nice, simple fix that'd be good for release branches and such, so we should put it in.  But then you should look closer at the current behavior and why it was done this way.  I'm pretty sure that what it currently does is wrong and should be changed.LayoutTests/fast/dom/Geolocation/script-tests/callback-to-deleted-context.js:12
 +      }, 100);
This seems like a recipe for flake.

LayoutTests/fast/dom/Geolocation/script-tests/callback-to-deleted-context.js:10
 +          testPassed('No callbacks invoked');
How do you know that?

LayoutTests/fast/dom/Geolocation/script-tests/callback-to-deleted-context.js:4
 +      iframe.src = 'resources/callback-to-deleted-context-inner2.html';
Isn't this a race?

[Alexey Proskuryakov]

> The call to DOMWindow::document() fails gracefully if the frame is detached, 

I'm not sure if we should rely on that. Logically, it makes no sense that one cannot easily get a document associated with DOMWindow when there is no frame, and there is a FIXME in DOMWindow::document() saying that we should fix that.

[Steve Block]

> I'm not sure if we should rely on that. Logically, it makes no sense that one
> cannot easily get a document associated with DOMWindow when there is no frame,
> and there is a FIXME in DOMWindow::document() saying that we should fix that.
OK, so I think the best solution is to use ActiveDOMObject as well as adding this check. I've been looking at the V8 bindings and it looks like an equivalent approach will be required there.

[Steve Block]

Created attachment 57984 [details]
Patch

[Jeremy Orlow]

Comment on attachment 57984 [details]
Patch

WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:59
 +      Frame* frame = V8Proxy::retrieveFrameForCurrentContext();
Just get the current context directly.

WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:78
 +      Frame* frame = V8Proxy::retrieveFrameForCurrentContext();
ditto

WebCore/bindings/v8/custom/V8CustomPositionCallback.cpp:72
 +      // Protect the script context until the callback returns.
Are you sure we need one of these?

WebCore/bindings/js/JSCustomPositionErrorCallback.cpp:48
 +      // ActiveDOMObject will null our pointer to the ScriptExecutionContext when
I'd lean towards not wrapping this comment.

WebCore/bindings/js/JSCustomPositionErrorCallback.cpp:50
 +      if (!scriptExecutionContext())
This is a good start, but ideally you'd be handling resume/suspend/stop instead of just detecting when the scriptExecutionContext has been destructed.

WebCore/ChangeLog:16
 +          The ScriptExecutionContext is ref'ed from script, so isn't destroyed until the
so _it_ isn't...

WebCore/ChangeLog:19
 +          accessing the Frame, so an additional check for the Frame is required.
Is any of this still relevant?

Overall, I think this change log description could be made more concise and not lose any interesting info.

WebCore/ChangeLog:21
 +          This change also prevents the V8 bindings from incorrectly holding a reference to the Frame.
We need to make sure this gets fixed in the other bindings.  Maybe hans or dumi would be interested in this, if you're not?

[Steve Block]

> Just get the current context directly.
Done

> WebCore/bindings/v8/custom/V8CustomPositionCallback.cpp:72
>  +      // Protect the script context until the callback returns.
> Are you sure we need one of these?
I can't see a need for it, but if the Frame needed protecting, I guess we need to protect the ScriptExecutionContext. This is what was done with a similar change to Database - http://trac.webkit.org/changeset/60330#file6

> I'd lean towards not wrapping this comment.
Done

> This is a good start, but ideally you'd be handling resume/suspend/stop instead of just detecting when the scriptExecutionContext has been destructed.
Yes, the reason I haven't done this here is that it's likely that the Geolocation object itself will end up being an ActiveDOMObject as part of Bug 34082. So we may not need the callbacks to also implement this behaviour if the Geolocation object is doing so anyway.

> WebCore/ChangeLog:16
>  +          The ScriptExecutionContext is ref'ed from script, so isn't
> destroyed until the
> so _it_ isn't...
Done

> WebCore/ChangeLog:19
>  +          accessing the Frame, so an additional check for the Frame is required.
> Is any of this still relevant?
Yes, I'm not aware of any current way to make the callbacks (for either V8 or JSC) without going through the Frame.

> Overall, I think this change log description could be made more concise and
> not lose any interesting info.
Done

> We need to make sure this gets fixed in the other bindings.  Maybe hans or
> dumi would be interested in this, if you're not?
I'm not aware of any other bindings that make this mistake. We already have Bug  40112 for the fact that Database holds onto the ScriptExecutionContext.

[Steve Block]

Created attachment 58038 [details]
Patch

[Jeremy Orlow]

Comment on attachment 58038 [details]
Patch

r=me

[Steve Block]

Comment on attachment 58038 [details]
Patch

Landed manually as r60840 http://trac.webkit.org/changeset/60840 with addition to GTK skipped list.

[Alexey Proskuryakov]

Sorry for not commenting earlier, it's a busy week.

I strongly dislike having bindings object inherit from ActiveDOMObject. These are not DOM objects regardless of how you slice it!

I think that a better way to fix this would be to factor out "request" objects associated with Geolocation object, and make them reference Geolocation. That way, the design would be closer XMLHttpRequest, and being similar to existing active objects can't do you any harm.

+    ScriptExecutionContext* context = globalObject()->scriptExecutionContext();
+    // We will fail to get the context if the frame has been detached.

That's pretty much the same as relying on DOMWindow::document() returning null. There isn't any deep reason why it's currently returning null.

[Jeremy Orlow]

(In reply to comment #13)
> Sorry for not commenting earlier, it's a busy week.
> 
> I strongly dislike having bindings object inherit from ActiveDOMObject. These are not DOM objects regardless of how you slice it!

Maybe we should change the name then?  ActiveDOMObject seems like the best way to get notifications about a ScriptExecutionContext's state changing.  (IDBRequest is another example of an object that inherits from ActiveDOMObject for the same reason.)
 
> I think that a better way to fix this would be to factor out "request" objects associated with Geolocation object, and make them reference Geolocation. That way, the design would be closer XMLHttpRequest, and being similar to existing active objects can't do you any harm.

Could you explain further what you're proposing?  I don't really understand (or know much about how XMLHttpRequest works).

[Alexey Proskuryakov]

My idea is:
- Geolocation can be a tiny object that implements the API (similar to Navigator);
- it won't be "active" in any way;
- whenever a request for position is made (either one shot or repeating), an object to handle this request is created. In fact, it will be similar to DOMTimer even more than to XMLHttpRequest;
- these request objects are ActiveDOMObjects. Like DOMTimers, they are not directly exposed to JS;
- I'm not really sure who should track granted permissions. Logically, it sounds like this is a job for SecurityOrigin.

Does this make sense? I think that the similarity to DOMTimer is very close - even down to watchers being represented as long IDs, like timers created with setInterval() are.

[Jeremy Orlow]

(In reply to comment #15)
> My idea is:
> - Geolocation can be a tiny object that implements the API (similar to Navigator);
> - it won't be "active" in any way;
> - whenever a request for position is made (either one shot or repeating), an object to handle this request is created. In fact, it will be similar to DOMTimer even more than to XMLHttpRequest;
> - these request objects are ActiveDOMObjects. Like DOMTimers, they are not directly exposed to JS;
> - I'm not really sure who should track granted permissions. Logically, it sounds like this is a job for SecurityOrigin.
> 
> Does this make sense? I think that the similarity to DOMTimer is very close - even down to watchers being represented as long IDs, like timers created with setInterval() are.

This seems like a very reasonable design, but what are the upsides of adding the extra layer to the design?

[Alexey Proskuryakov]

Mostly, ease of understanding and refactoring. Not long ago, DOMTimer code was all in DOMWindow, and we had to factor it out to more easily fix bugs, and to support timers in workers. Geolocation design is pretty much identical, so solving the same problems differently isn't good.

[Jeremy Orlow]

(In reply to comment #17)
> Mostly, ease of understanding and refactoring. Not long ago, DOMTimer code was all in DOMWindow, and we had to factor it out to more easily fix bugs, and to support timers in workers. Geolocation design is pretty much identical, so solving the same problems differently isn't good.

I agree that solving the problem in two ways is not good, but it seems as though we do it all over the place in WebKit and historically we haven't put too much emphasis on cleaning up the worse ways of doing things.  When you refactor code, there is the risk of introducing new bugs and there is of course opportunity cost.  That said, I think a strategically placed FIXME (so the next time someone is writing code based on the old code they get a pointer towards the better way of doing something) is the very least you should do in such a case.

Anyway, I was talking to Steve and he was telling me that there is a lot of code in GeoLocation that's kind of a mess.  For example, there are two interfaces.  And it's pretty clear now that the second one is far superior and that the first should be merged into it.  He also seemed to think the code would be a bit cleaner if the code was factored as you suggested.  And I think I have him convinced the callbacks should handle suspend/resume more gracefully.  That on top of the fact that Steve's already waist deep in GeoLocation code, it seems like it might make sense for him to just finish the job.