Bug 37725 – FrameLoader::clear() clears runtime objects that cached pages later rely on

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Robert Hogan]

# Related failure to https://bugs.webkit.org/show_bug.cgi?id=31626
# Navigating to a cached page with javascript history.back() does not restore the JS context.
# CONSOLE MESSAGE: line 28: ReferenceError: Trying to access object from destroyed plug-in.
# media/restore-from-page-cache.html is also affected by this issue in addition to the issue
# in bug 31626.

This bug is a fairly close relation of https://bugs.webkit.org/show_bug.cgi?id=31626

These tests all fail:

fast/events/pageshow-pagehide-on-back-cached.html
fast/events/pageshow-pagehide-on-back-cached-with-frames.html
fast/loader/input-element-page-cache-crash.html
fast/dom/Window/timer-resume-on-navigation-back.html
loader/go-back-to-different-window-size.html

with the error:

CONSOLE MESSAGE: line 28: ReferenceError: Trying to access object from destroyed plug-in.

This error is thrown by RuntimeObject::getOwnPropertySlot() (runtime_object.cpp) when the JS runtime object has no JS Instance.

These tests all have this error for the same reason: they are attempting to call layoutTestController on a page that has been retrieved from WebCore's page cache. Specifically, the cached page is retrieved following a history.back() JS call from the test.

So when FrameLoader is restoring a page from the cache it follows this sequence (from the bottom up):

#0  0x00e653d0 in JSC::Bindings::RuntimeObject::invalidate() ()
#1  0x00e675a3 in JSC::Bindings::RootObject::invalidate() ()
#2  0x00e35877 in WebCore::ScriptController::clearScriptObjects() ()
#3  0x011e691e in WebCore::FrameLoader::clear(bool, bool, bool) ()
#4  0x011f0f9d in WebCore::FrameLoader::open(WebCore::CachedFrameBase&) ()
#5  0x010c49a5 in WebCore::CachedFrame::open() ()
#6  0x010c640a in WebCore::CachedPage::restore(WebCore::Page*) ()
#7  0x011f0d9c in WebCore::FrameLoader::open(WebCore::CachedPage&) ()
#8  0x011eff49 in WebCore::FrameLoader::commitProvisionalLoad(WTF::PassRefPtr<WebCore::CachedPage>) ()
#9  0x011f4e1a in WebCore::FrameLoader::loadProvisionalItemFromCachedPage() ()

clearScriptObjects() and the following calls set the m_rootObjects and the m_bindingRootObject associated with the frame's scriptController to 0. This is normal behaviour prior to loading a page.

During a normal page load (i.e. a non-cached page) this same call to clearScriptObjects() is shortly followed by dispatchDidClearWindowObjectsInAllWorlds() in FrameLoader. In the case of the DRT this prompts the DRT to add objects like the layoutTestController to the cleared JSDOMWindow associated with the frame.

In the case where a cached page is loaded this doesn't (or certainly shouldn't) need to happen. The runtimeObject added to the cached page should still be present in the JSDOMWindow. This is how Qt adds the layoutTestController to the frame:

    JSC::JSLock lock(JSC::SilenceAssertionsOnly);
    JSDOMWindow* window = toJSDOMWindow(d->frame, mainThreadNormalWorld());
    JSC::Bindings::RootObject* root = d->frame->script()->bindingRootObject();
    JSC::ExecState* exec = window->globalExec();
    JSC::JSObject* runtimeObject =
            JSC::Bindings::QtInstance::getQtInstance(object, root, ownership)->createRuntimeObject(exec);
    JSC::PutPropertySlot slot;
    window->put(exec, JSC::Identifier(exec, (const UChar *) name.constData(), name.length()), runtimeObject, slot);

So as long as the JSDomWindow is cached with the frame it should work, and it is - at least it appears to be since CachedFrame stores the frame->view() along with other bits and pieces:

CachedFrameBase::CachedFrameBase(Frame* frame)
    : m_document(frame->document())
    , m_documentLoader(frame->loader()->documentLoader())
    , m_view(frame->view())
    , m_mousePressNode(frame->eventHandler()->mousePressNode())
    , m_url(frame->loader()->url())
    , m_isMainFrame(!frame->tree()->parent())

CachedFrame::CachedFrame(Frame* frame)
    : CachedFrameBase(frame)
{
    // Active DOM objects must be suspended before we cached the frame script data
    m_document->suspendActiveDOMObjects();
    m_cachedFrameScriptData.set(new ScriptCachedFrameData(frame));
    // Custom scrollbar renderers will get reattached when the document comes out of the page cache
    m_view->detachCustomScrollbars();
    m_document->documentWillBecomeInactive(); 
    frame->clearTimers();
    m_document->setInPageCache(true);
..
}

and the JSDomWindow is stored in the m_cachedFrameScriptData:

    ScriptController* scriptController = frame->script();
    ScriptController::ShellMap& windowShells = scriptController->m_windowShells;

    ScriptController::ShellMap::iterator windowShellsEnd = windowShells.end();
    for (ScriptController::ShellMap::iterator iter = windowShells.begin(); iter != windowShellsEnd; ++iter) {
        JSDOMWindow* window = iter->second->window();
        m_windows.add(iter->first.get(), window);
        m_domWindow = window->impl();
    }

and restored here:

void ScriptCachedFrameData::restore(Frame* frame)
{
    JSLock lock(SilenceAssertionsOnly);

    ScriptController* scriptController = frame->script();
    ScriptController::ShellMap& windowShells = scriptController->m_windowShells;

    ScriptController::ShellMap::iterator windowShellsEnd = windowShells.end();
    for (ScriptController::ShellMap::iterator iter = windowShells.begin(); iter != windowShellsEnd; ++iter) {
        DOMWrapperWorld* world = iter->first.get();
        JSDOMWindowShell* windowShell = iter->second.get();

        if (JSDOMWindow* window = m_windows.get(world))
            windowShell->setWindow(window);
        else {
            windowShell->setWindow(frame->domWindow());

            if (Page* page = frame->page()) {
                scriptController->attachDebugger(windowShell, page->debugger());
                windowShell->window()->setProfileGroup(page->group().identifier());
            }
        }
    }
}

Debugging shows the window is retrieved from:

        if (JSDOMWindow* window = m_windows.get(world))
            windowShell->setWindow(window);

So all seems fine in WebCore except it doesn't work for QtWebKit. That would suggest there is something wrong with the way QtInstance works. But it's hard to see what if anything QtInstance is doing wrong - more to the point it's hard to see how the runtime object or frame->script()->bindingRootObject() survive when they're cached, as invalidate() completely zaps them and this happens entirely in WebCore. Setting the runtime objects and the root objects to zero results in them being destroyed. Maybe the fact that the JSDomWindow is cached in a protected pointer:

        typedef HashMap< RefPtr<DOMWrapperWorld>, JSC::ProtectedPtr<JSDOMWindow> > JSDOMWindowSet;

actually prevents this zapping. But if that's the case then why doesn't it work for QtWebKit too?

So my guess is that the problem is probably in QtInstance but I can't see what.

[Robert Hogan]

Anders, would you have any pointers on this?

[Robert Hogan]

Created attachment 53869 [details]
Patch

Updated docs as well!

[Simon Hausmann]

(In reply to comment #2)
> Created an attachment (id=53869) [details]
> Patch
> 
> Updated docs as well!

I think by accident you attached the patch to the wrong report. Did you mean bug 37324 ?

[Robert Hogan]

Comment on attachment 53869 [details]
Patch

Wrong bug!

[Robert Hogan]

Created attachment 56777 [details]
Patch

[Robert Hogan]

This same problem occurs in GTK and Chromium (though Chromium doesn't care because of the way it uses the WebCore cache). 

The only explanation I can think of why it isn't a problem for Mac and Windows is that they are not using the same frame for each navigation.

If there is a security problem with keeping the same JS script objects across navigations in the same frame then this fix is wrong.

[Robert Hogan]

Created attachment 56911 [details]
Patch

[Robert Hogan]

(In reply to comment #7)
> Created an attachment (id=56911) [details]
> Patch

I'm pretty sure that this isn't the right way to solve the problem but it does so without any regressions to layout tests. Looking forward to the input of someone who knows a lot more about this than I do.

[Robert Hogan]

Created attachment 56912 [details]
Patch

[Alexey Proskuryakov]

See also: bug 34679. The question is what's different on Qt that also makes these tests fail.

> If there is a security problem with keeping the same JS script objects
> across navigations in the same frame then this fix is wrong.

JSDOMWindowShell should remain, JSDOMWindow should be replaced (and it goes into b/f cache together with Document).

[Robert Hogan]

(In reply to comment #10)
> See also: bug 34679. The question is what's different on Qt that also makes these tests fail.

To be honest, my question is how this works at all for Mac or Windows. (It fails for GTK and is an unknown quantity in Chromium, which skips all these tests). clearScriptObjects() completely zaps the runtimeObjects and the bindingRootObject associated with the JSDOMWindow. So how it survives in Mac/Windows is a mystery to me since although JSDomWindow is preserved in the cache, when restored its runtimeObject should no longer exist if invalidate() works properly.

I'm not very au fait with the protector() and ProtectedPtr<> stuff but is it possible that the Mac/Windows is casting some protected charm on the runTimeObjects that prevents them getting destroyed while the cache still references them? That seems unlikely from what I can understand of the code, but it's all I've got.

> 
> > If there is a security problem with keeping the same JS script objects
> > across navigations in the same frame then this fix is wrong.
> 
> JSDOMWindowShell should remain, JSDOMWindow should be replaced (and it goes into b/f cache together with Document).

So the patch is definitely wrong, removing the review flag.

[Robert Hogan]

Created attachment 57136 [details]
Patch

[Robert Hogan]

(In reply to comment #12)
> Created an attachment (id=57136) [details]
> Patch

This one just detaches the objects instead of retaining them completely. Is that better?

[Alexey Proskuryakov]

Obviously, we should look for a fix that doesn't change any cross-platform code. Is there anything specific you'd like me to test on Mac, or any other specific information you need?

[WebKit Review Bot]

Attachment 57136 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2514062

[Robert Hogan]

(In reply to comment #14)
> Obviously, we should look for a fix that doesn't change any cross-platform code. Is there anything specific you'd like me to test on Mac, or any other specific information you need?

Yes, Alexey that would be great.

First of all, need to ensure that Mac is actually calling invalidate() when navigating to a new page: so it would be good to set a breakpoint in JSC::Bindings::RuntimeObject::invalidate() and run DRT in gdb and make a note of the value of the m_instance that gets invalidated.

Assuming invalidate() is called, you could then set a breakpoint in RuntimeObject::getOwnPropertySlot() and see what the value of m_instance is when it is hit just after a cached page has been loaded.

The thing I would like to know if you see an m_instance get invalidated in invalidate() but subsequently see it used in getOwnPropertySlot() just after a cached page has been loaded (this should happen just after invalidate() has been called with the following frame stack:

#0  0x00e653d0 in JSC::Bindings::RuntimeObject::invalidate() ()
#1  0x00e675a3 in JSC::Bindings::RootObject::invalidate() ()
#2  0x00e35877 in WebCore::ScriptController::clearScriptObjects() ()
#3  0x011e691e in WebCore::FrameLoader::clear(bool, bool, bool) ()
#4  0x011f0f9d in WebCore::FrameLoader::open(WebCore::CachedFrameBase&) ()
#5  0x010c49a5 in WebCore::CachedFrame::open() ()
#6  0x010c640a in WebCore::CachedPage::restore(WebCore::Page*) ()
).

[Alexey Proskuryakov]

This bug actually affects Mac, as well, but to a different extent. I can explain the differences now.

Mac DRT installs layoutTestController via JavaScriptCore API (JSObjectMake/JSObjectSetProperty). So, it is not a RuntimeObject, WebCore doesn't know about it, and it isn't invalidated by ScriptController::clearScriptObjects(). But Mac DRT installs some other controllers via an old WebScriptObject API, and trying to use e.g. eventSender on a restored page causes the same problem you see on Qt.

Most RuntimeObjects come from Java and plug-ins, and they do need to be invalidated by clearScriptObjects(). But Objective C and Qt runtime objects probably needn't (or at least, that invalidation should be automatically reversed when the page come from b/f cache).

So, we need to clean up how plug-ins interact with caching:
- NPAPI plug-ins currently prevent pages from going to b/f cache, but this should be fixed. In the future, plug-ins will be stopped, and CRuntimeObject objects will still be invalidated;
- Objective C plug-ins ("WebKit plug-ins" on Mac OS X) will still prevent pages from going to b/f cache;
- Objective C objects created via WebScriptObject probably needn't be affected by pages going to and from b/f cache;
- same for Qt bindings objects;
- I'm not sure if Java prevents pages from going to b/f cache, or whether it should (is there a way to ask VM to pause applets?);

This can be achieved by making invalidate() virtual, and having different implementations. Or this can be achieved by not even tracking some objects in RuntimeRoot. I don't have an answer about which is best - RuntimeRoot has other responsibilities besides invalidating objects (e.g. it's GC protecting them, which is not necessary for window object properties, but is necessary in general). Also, I'm not sure why it's OK for RootObject itself to be invalidated. This needs to be investigated.

I don't understand why these tests fail on Gtk - I thought it used cross-platform DRT code that relies on JSC API for all controllers. So, Gtk may have a different issue.

[Alexey Proskuryakov]

Comment on attachment 57136 [details]
Patch

I'm not sure about this patch. It might be correct, as long as NPAPI plug-ins prevent pages from going into b/f cache. And it might even be a step in the right direction regardless of whether they do - we can track NPAPI objects separately for invalidation.

I think that the best way to move forward would be to investigate this deeper - hopefully my comments above will help.

[Robert Hogan]

> 
> So, we need to clean up how plug-ins interact with caching:
> - NPAPI plug-ins currently prevent pages from going to b/f cache, but this should be fixed. In the future, plug-ins will be stopped, and CRuntimeObject objects will still be invalidated;
> - Objective C plug-ins ("WebKit plug-ins" on Mac OS X) will still prevent pages from going to b/f cache;
> - Objective C objects created via WebScriptObject probably needn't be affected by pages going to and from b/f cache;
> - same for Qt bindings objects;
> - I'm not sure if Java prevents pages from going to b/f cache, or whether it should (is there a way to ask VM to pause applets?);
> 

I think each of the above currently applies - re Java, createJavaAppletWidget() in FrameLoader.cpp sets m_containsPlugins to true so it is treated the same as an NPAPI plugin. 

So plugins-wise only pages with Qt bindings and Objective C objects get cached therefore it seems appropriate to just detach any runtimeObjects in such cases.

(In reply to comment #18)
> (From update of attachment 57136 [details])
> I'm not sure about this patch. It might be correct, as long as NPAPI plug-ins prevent pages from going into b/f cache. 

They do:

bool FrameLoader::canCachePageContainingThisFrame()
{
<..>
    return m_documentLoader
        && m_documentLoader->mainDocumentError().isNull()
        // FIXME: If we ever change this so that frames with plug-ins will be cached,
        // we need to make sure that we don't cache frames that have outstanding NPObjects
        // (objects created by the plug-in). Since there is no way to pause/resume a Netscape plug-in,
        // they would need to be destroyed and then recreated, and there is no way that we can recreate
        // the right NPObjects. See <rdar://problem/5197041> for more information.
        && !m_containsPlugIns


> This can be achieved by making invalidate() virtual, and having different implementations. 

This certainly sounds necessary when it comes to stopping plugins and then caching them. I think the capability is there to do that now isn't it? 

>Or this can be achieved by not even tracking some objects in RuntimeRoot. I don't have an answer about which is best - RuntimeRoot has other responsibilities besides invalidating objects (e.g. it's GC protecting them, which is not necessary for window object properties, but is necessary in general). 

Detaching the objects is the only solution I've got my head around at the moment. Is it an acceptable solution for fixing the immediate problem?

[Alexey Proskuryakov]

>> I'm not sure about this patch. It might be correct, as long as NPAPI plug-ins prevent 
>> pages from going into b/f cache. 

> They do:

What I was saying is that this is something we want to change.

[Robert Hogan]

(In reply to comment #20)
> >> I'm not sure about this patch. It might be correct, as long as NPAPI plug-ins prevent 
> >> pages from going into b/f cache. 
> 
> > They do:
> 
> What I was saying is that this is something we want to change.

And I was trying to wimp out of doing that under this bug. It does seem like stopping plugins and caching the page that contains is something that can be done now, so I'll give it a go here.

[Alexey Proskuryakov]

It wasn't my intention to make you implement caching of pages with plug-ins. I just wanted to make sure that your fix doesn't make it harder for someone in the future.

But you you want to tackle this, it will be much appreciated, of course. We're tracking this problem as bug 13634.

[Brady Eidson]

(In reply to comment #21)
> (In reply to comment #20)
> > >> I'm not sure about this patch. It might be correct, as long as NPAPI plug-ins prevent 
> > >> pages from going into b/f cache. 
> > 
> > > They do:
> > 
> > What I was saying is that this is something we want to change.
> 
> And I was trying to wimp out of doing that under this bug. It does seem like stopping plugins and caching the page that contains is something that can be done now, so I'll give it a go here.

I think that task is much more complicated than you realize, and it is not related to resolving this bug.

(In reply to comment #22)
> It wasn't my intention to make you implement caching of pages with plug-ins. I just wanted to make sure that your fix doesn't make it harder for someone in the future.
> 
> But you you want to tackle this, it will be much appreciated, of course. We're tracking this problem as bug 13634.

What Alexey said - if you have thoughts about caching pages with plug-ins, please jot them down in 13634.  But that task is independent of this one.  Please don't conflate the two.

[Robert Hogan]

(In reply to comment #23)
> I think that task is much more complicated than you realize, and it is not related to resolving this bug.

Quickly realized that! I understand what Alexey was getting at a little better now.

(In reply to comment #23)
>Also, I'm not sure why it's OK for RootObject itself to be invalidated. This needs to be investigated.

Is there merit in creating a special case of m_bindingRootObject that is only used for Qt bindings object and ObjC bindings objects? And this special case would not get invalidated in clearScriptObjects() so its RuntimeObjects would remain tracked in the root object until the frame gets destroyed?

ObjC plugins could continue to use m_bindingRootObject and that would continue to get invalidated as required.

This would remove the need to worry about how/when to destroy the RuntimeObjects later, which I think is a concern with virtualizing RuntimeObject::invalidate() instead.

[Robert Hogan]

Created attachment 58035 [details]
Patch

[Alexey Proskuryakov]

+    // The root object used for objects bound outside the context of a plugin, such
+    // as Objective-C plugins. These objects prevent a page from being cached so are
+    // safe to invalidate() when WebKit navigates away from the page that contains them.

Objective-C isn't a great example here, because we cannot tell an object added by an Objective-C plug-in from an object added by an embedder (such as those other controllers added by Mac DumpRenderTree). NPAPI would be a better example - even when those won't prevent caching, they will still be stopped when navigating away.

This patch looks good to me, but the bindings code scares me so much that I'd like someone else to double check. Everyone I think of is already CC'ed on this bug.

[Robert Hogan]

Created attachment 58403 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 58403 [details]
Patch

WebCore/bindings/js/ScriptController.h:201
 +      // safe to invalidate() when WebKit navigates away from the page that contains them.
It is not the objects that prevent page caching, it is the plugin it self.

[Alexey Proskuryakov]

Comment on attachment 58403 [details]
Patch

WebCore/bindings/js/ScriptController.h:205
 +      // a page from being cached they are not invalidated.
Again, it is not the bound objects that don't prevent the page caching, it is the use of those objects.  I also don't think it is necessary to call out Qt-bindings objects explicitly, it won't be helpful to future readers of the code.


Please fix these comments before landing.

[Alexey Proskuryakov]

(the above two comments were from Sam Weinig, who also thinks it's a good patch).

[Robert Hogan]

Landed as http://trac.webkit.org/changeset/61062

Regression to tst_qwebframe tracked at:
https://bugs.webkit.org/show_bug.cgi?id=40527

http/tests/misc/font-face-in-multiple-segmented-faces.html crashed on buildbot, but appears to be unrelated. Can't reproduce locally.

[Robert Hogan]

Landed http://trac.webkit.org/changeset/61062

[Simon Hausmann]

*** Bug 31626 has been marked as a duplicate of this bug. ***

[Darin Adler]

(In reply to comment #26)
> +    // The root object used for objects bound outside the context of a plugin, such
> +    // as Objective-C plugins. These objects prevent a page from being cached so are
> +    // safe to invalidate() when WebKit navigates away from the page that contains them.
> 
> Objective-C isn't a great example here, because we cannot tell an object added by an Objective-C plug-in from an object added by an embedder (such as those other controllers added by Mac DumpRenderTree). NPAPI would be a better example - even when those won't prevent caching, they will still be stopped when navigating away.
> 
> This patch looks good to me, but the bindings code scares me so much that I'd like someone else to double check. Everyone I think of is already CC'ed on this bug.

We’ve now got a report of this same issue with Objective-C objects added by an embedder. This is Apple internal <rdar://problem/8547940>.