Bug 5727 – We want to evaluate scripts in viewless documents

• Component: DOM
• Status: RESOLVED INVALID

[Anders Carlsson]

For xbl, hyatt says that we need to be able to evaluate scripts in viewless documents

[Julien Chaffraix]

Created attachment 21540 [details]
Draft - No test case (will be added on the final version) - Should also have used 'svn mv'

This is a first draft. It is based on the empty client work around (see https://bugs.webkit.org/show_bug.cgi?id=5971) and adds a dummy Page / Frame / FrameView hierarchy to use for viewless element.

I am particularly concerned about the security issues that could arise from this approach (but lacks the knowledge to assess them). Especially since it adds the possibility to run some JavaScript using our dummy hierarchy.

Any comments are welcome.

[Darin Adler]

Comment on attachment 21540 [details]
Draft - No test case (will be added on the final version) - Should also have used 'svn mv'

Factoring out the empty clients is great. Maybe it would be best to do that in a separate check-in, first.

Why does the setDummyFrame function have a "shouldEnableJavaScript" boolean parameter? All the callers pass true.

I'm not sure setDummyFrame should be a Document member function. It seems to me that this is more of a controller operation, and we should try to avoid adding more controller functions into the model object, Document.

Perhaps it should just be a static member of FrameLoader that takes a Document argument.

The fakeLoad function definitely should *not* be added to Frame. This clearly belongs on FrameLoader.

The dummyFrame function should return PassRefPtr<Frame> rather than just a Frame*. I think the function's name should have a verb in it, such as createDummyFrame. While it's marginally OK to have this on Frame, I think this function, too, would be better on FrameLoader, which has the responsibility to create new frames. The Frame class itself is just the "hub" of Frame features, and should have very little code in it.

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 21540 [details] [edit])
> Factoring out the empty clients is great. Maybe it would be best to do that in
> a separate check-in, first.

Good idea, I will split the patch.

> Why does the setDummyFrame function have a "shouldEnableJavaScript" boolean
> parameter? All the callers pass true.

You have missed one: SVGImage does not need JavaScript and thus does not pass true but uses the default (that is false).
Some elements need a Frame to load a remote resource (using fakeLoad) and may not need access to JavaScript (that was the previous behaviour that I try to keep using this parameter).

> I'm not sure setDummyFrame should be a Document member function. It seems to me
> that this is more of a controller operation, and we should try to avoid adding
> more controller functions into the model object, Document.
> 
> Perhaps it should just be a static member of FrameLoader that takes a Document
> argument.

Currently Document has no Frame setter so that's why I had to put in Document. I guess there is a reason why we did not want to be able to modify a Document's Frame (that's why I add this particular setter and not a setFrame() method).

A solution would be to add a private setFrame() to Document and add a friend method Frame::setDummyFrameFor(Document*).

> The fakeLoad function definitely should *not* be added to Frame. This clearly
> belongs on FrameLoader.

I will change that.

> The dummyFrame function should return PassRefPtr<Frame> rather than just a
> Frame*. 

I will correct that too.

> I think the function's name should have a verb in it, such as
> createDummyFrame. While it's marginally OK to have this on Frame, I think this
> function, too, would be better on FrameLoader, which has the responsibility to
> create new frames. The Frame class itself is just the "hub" of Frame features,
> and should have very little code in it.

For this one, I would tend to prefer it on Frame as we are returning a Frame (or a PassRefPtr<Frame>). IMHO it is also more logical to call it with Frame::createDummyFrame() than FrameLoader::createDummyFrame().

[Julien Chaffraix]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 21540 [details] [edit] [edit])
> > Factoring out the empty clients is great. Maybe it would be best to do that in
> > a separate check-in, first.
> 
> Good idea, I will split the patch.
> 

Filed bug 19529 for the refactoring part.

[Julien Chaffraix]

Created attachment 21870 [details]
2nd version - updated with Darin's comments

[Darin Adler]

Comment on attachment 21870 [details]
2nd version - updated with Darin's comments

Here's better wording for this comment:

+    // Changing the Document frame should not occurs under normal condition.
+    // Currently only the dummyFrame issue requires it.

    // Changing the Document frame should not occur under normal conditions.
    // Currently only createDummyFrame requires it.

And since this function will be rarely used, it should be moved farther down in the source file. The most used functions should be up at the top if possible.

This seems to be indented incorrectly:

+    if (!doc->frame())
+            Frame::setDummyFrameFor(doc, true);

In the createDummyFrame function:

+    Page* page = new Page(dummyChromeClient, dummyContextMenuClient, dummyEditorClient, dummyDragClient, dummyInspectorClient);

What owns this page? It seems to me that this Page object leaks permanently. This is the most serious problem with the patch.

+    Frame* frame = new Frame(page, 0, dummyFrameLoaderClient);
+    [...]
+    return adoptRef(frame);

The correct style is to do adoptRef right on the same line as the "new" and keep it in a RefPtr. So it should be:

    RefPtr<Frame> frame = adoptRef(new Frame(page, 0, dummyFrameLoaderClient));
    [...]
    return frame.release();

We want the new and the adoptRef to be as close to each other as possible.

How about skipping the local variable here:

+    FrameView* frameView = new FrameView(frame);
+    frame->setView(frameView);

Do this instead:

    frame->setView(new FrameView(frame));

This reads strangely to me:

    frame->d->m_loader.init(false);

Please consider the more usual:

    frame->loader()->init(false);

I think the right thing to do is to put the Document::setFrame function call into createDummyFrame. Everyone will call that function directly. Simply have it attach the dummy frame to the document automatically if you passed a document into createDummyFrame. Then we can get rid of the awkwardly name setDummyFrameFor.

+    static void setDummyFrameFor(Document*, bool shouldEnableJavaScript);

I still think that createDummyFrame belongs in FrameLoader, but I guess you weren't convinced last time I suggested it.

For this:

+        Document* doc = document();
+        if (!doc->frame())

the style I now prefer is:

    Document* document = this->document();
    if (!document->frame())

I don't like using various mixed abbreviations just to dodge member names.

I still don't see any callers passing false for shouldEnableJavaScript to setDummyFrameFor, despite your earlier comments. Is that coming in a future patch?

review- because of the Page ownership issue.

I'd also like you to refine the arguments to createDummyFrame and its behavior further and consider my other comments. But the only essential issue is the ownership one.

[Julien Chaffraix]

[Removed other comments that I will tackle]

> In the createDummyFrame function:
> 
> +    Page* page = new Page(dummyChromeClient, dummyContextMenuClient,
> dummyEditorClient, dummyDragClient, dummyInspectorClient);
> 
> What owns this page? It seems to me that this Page object leaks permanently.
> This is the most serious problem with the patch.

You are right. The Frame owns the Frame-FrameView-Page hierarchy and thus should clean them. I will make the modifications to ensure that.

> I think the right thing to do is to put the Document::setFrame function call
> into createDummyFrame. Everyone will call that function directly. Simply have
> it attach the dummy frame to the document automatically if you passed a
> document into createDummyFrame. Then we can get rid of the awkwardly name
> setDummyFrameFor.
> 
> +    static void setDummyFrameFor(Document*, bool shouldEnableJavaScript);
> 
> I still think that createDummyFrame belongs in FrameLoader, but I guess you
> weren't convinced last time I suggested it.

It seems strange but I could live with it.

> I still don't see any callers passing false for shouldEnableJavaScript to
> setDummyFrameFor, despite your earlier comments. Is that coming in a future
> patch?

There is none for setDummyFrameFor but there is one for createDummyFrame. Here is the code snippet:

Index: WebCore/svg/graphics/SVGImage.cpp
===================================================================
--- WebCore/svg/graphics/SVGImage.cpp	(revision 34713)
+++ WebCore/svg/graphics/SVGImage.cpp	(working copy)
[...]
-        m_page->settings()->setJavaScriptEnabled(false);
[...]
+        m_frame = Frame::createDummyFrame(false);

This was the original behaviour that I wanted to keep.

I do not intend to do any modification on the caller's argument so there will be only 2 use cases: we provide a Document because we need to execute JavaScript or we do not provide one because we just want to load some resources.
I guess we could use the Document's argument to enable JavaScript or not.

[Julien Chaffraix]

Created attachment 21939 [details]
3nd attempt - Correct minor issues and should not leak the Page

[Eric Seidel (no email)]

Comment on attachment 21939 [details]
3nd attempt - Correct minor issues and should not leak the Page

+    void setFrame(Frame* frame) { m_frame = frame; }

should probably be private.  FrameLoader could be a friend class then (if it isn't already).

+        Document* document = this->document();

Just call it "doc", then you don't need to use this-> all over.

You seem to be fixing SVG image transparency issues here too?  That sounds like a test case is in order...

Seems like +        bool m_isDummyFrame; should be grouped with the other bools.

Darin should see this again.

[Eric Seidel (no email)]

Comment on attachment 21939 [details]
3nd attempt - Correct minor issues and should not leak the Page

r- for my above comments.  If you're fixing an SVG issue, you need a separate SVG test (and that change should really go in a separate patch).

[Julien Chaffraix]

Adding Eric so that he could see the forthcoming question.

[Julien Chaffraix]

(In reply to comment #9)
> (From update of attachment 21939 [details] [edit])
> 
> You seem to be fixing SVG image transparency issues here too?  That sounds like
> a test case is in order...

I have though about that a few times and I fail to see which SVG issue I could have fixed, could you provide an example of what kind of issue should be fixed so that I could test and understand?

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #9)
> > (From update of attachment 21939 [details] [edit] [edit])
> > 
> > You seem to be fixing SVG image transparency issues here too?  That sounds like
> > a test case is in order...
> 
> I have though about that a few times and I fail to see which SVG issue I could
> have fixed, could you provide an example of what kind of issue should be fixed
> so that I could test and understand?

I was just confused.  That's all. :)  I also don't see any SVG-related code in this patch. 
> 

[Julien Chaffraix]

(In reply to comment #9)
> (From update of attachment 21939 [details] [edit])
> +    void setFrame(Frame* frame) { m_frame = frame; }
> 
> should probably be private.  FrameLoader could be a friend class then (if it
> isn't already).

I would rather not to. It would break the abstraction between Document and FrameLoader and I do not see any advantages over a public method with a warning.

> 
> +        Document* document = this->document();
> 
> Just call it "doc", then you don't need to use this-> all over.

Originally I called it doc but Darin asked me to change to that. The 2 notations are used in WebCore but this one is more coherent with our coding style (though a bit heavy here).

> Seems like +        bool m_isDummyFrame; should be grouped with the other
> bools.
> 
You're right.

[Julien Chaffraix]

Created attachment 23746 [details]
Next iteration: updated to ToT, should address the lastest comments

[Eric Seidel (no email)]

Comment on attachment 23746 [details]
Next iteration: updated to ToT, should address the lastest comments

This:
 5175     if (document) {
 5176         // If a document was provided we need to initialize
 5177         // the loader but should not create an empty document.
 5178         frame->setDocument(document);
 5179         frame->loader()->init(false);
 5180     } else
 5181         frame->init();
 5182 
 5183     if (document)
 5184         document->setFrame(frame.get());
can be one unified if block.  There is no need for a second if (document) block there.

I don't think there shoudl be a setIsDummyFrame call.  Instead, there should be a Frame::create() version which creates a dummy frame.

Does a frame ever need the ability to become a non-dummy?

Otherwise this looks great.

[Julien Chaffraix]

(In reply to comment #16)
> (From update of attachment 23746 [details] [edit])
> This:
>  5175     if (document) {
>  5176         // If a document was provided we need to initialize
>  5177         // the loader but should not create an empty document.
>  5178         frame->setDocument(document);
>  5179         frame->loader()->init(false);
>  5180     } else
>  5181         frame->init();
>  5182 
>  5183     if (document)
>  5184         document->setFrame(frame.get());
> can be one unified if block.  There is no need for a second if (document) block
> there.

Right, I will change that.

> 
> I don't think there shoudl be a setIsDummyFrame call.  Instead, there should be
> a Frame::create() version which creates a dummy frame.

Right here too (I got confused in the first version and forgot to change that).

> Does a frame ever need the ability to become a non-dummy?

Nope, once it is created, we should not be able to modify it. I will change that too.

Thanks! I will attach the new version here for another round of review.

[Julien Chaffraix]

> > I don't think there shoudl be a setIsDummyFrame call.  Instead, there should be
> > a Frame::create() version which creates a dummy frame.
> 
> Right here too (I got confused in the first version and forgot to change that).

In fact this was a request from Darin to avoid adding code to the Frame (see comment 2) so I think I will stick to it.

[Julien Chaffraix]

Committed in r37060 with the following changes:
- left the createDummyFrame in FrameLoader (asked by Darin)
- removed the bool parameter in setIsDummyFrame as we do not need to switch from a dummy frame to a normal one.

(the changes were too small to do another round of review)

[Mark Rowe (bdash)]

This was rolled out in r37112 as it introduced a large number of leaks on the layout tests.  You can easily reproduce these by doing "run-webkit-tests --leaks fast/backgrounds" with a debug build of WebKit.  With the patch applied I see over 2,200 leaks from that small set of tests.  After reverting the patch I was down to a much smaller number.

[Eric Seidel (no email)]

Comment on attachment 23746 [details]
Next iteration: updated to ToT, should address the lastest comments

Marking r- since this was rolled out.

[Julien Chaffraix]

Created attachment 25249 [details]
Fixed version - Should not leak

Ok, it took me a while but I have finally understood the way to go: we should create a dummyPage that holds the whole hierarchy and is responsible for cleaning.

As a side note, I have added the createDummyPage to the Page but I am ok with moving it to a more appropriate place.

[Julien Chaffraix]

And this time I have tested it with --leaks with no abnormal increase in the number of leaks. 

[Eric Seidel (no email)]

Comment on attachment 25249 [details]
Fixed version - Should not leak

You could ASSERT here that the page is a dummy page:
80     // Changing the Document frame should not occur under normal conditions.
 781     // Currently only used by the dummyPage mechanism.
 782     void setFrame(Frame* frame) { m_frame = frame; }

You should ASSERT(document->frame()) after:
     if (!document->frame())
142          return;
 141         dummyPage.set(Page::createDummyPage(document));


We're allowing Scripts to execute in viewless documents even though the user/embedder could have turned them off via settings:

3     // Giving a document implies that we need to execute JavaScript.
 644     page->settings()->setJavaScriptEnabled(document);

I think dummy page might be the wrong model.  We may need a way to always get to a page, even if you're not in a frame.  And use that way when executing scripts.  We can't allow scripts to execute when embedders have turned them off.

[Adam Klein]

Just found this while reading through ScriptElement.cpp, and got a little worried since the proposed HTMLTemplateElement relies on scripts in viewless documents not running. Perhaps this bug is obsolete, since the reason for needing this was XBL, and XBL is dead?

[Blaze Burg]

Per prior comment, I think this bug is stale. Please reopen if not the case.