Bug 5727 - We want to evaluate scripts in viewless documents
: We want to evaluate scripts in viewless documents
Status: REOPENED
: WebKit
HTML DOM
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2005-11-13 10:14 PST by
Modified: 2012-09-26 17:29 PST (History)


Attachments
Draft - No test case (will be added on the final version) - Should also have used 'svn mv' (62.59 KB, patch)
2008-06-06 19:52 PST, Julien Chaffraix
darin: review-
Review Patch | Details | Formatted Diff | Diff
2nd version - updated with Darin's comments (14.82 KB, patch)
2008-06-21 06:26 PST, Julien Chaffraix
darin: review-
Review Patch | Details | Formatted Diff | Diff
3nd attempt - Correct minor issues and should not leak the Page (17.51 KB, patch)
2008-06-25 16:43 PST, Julien Chaffraix
eric: review-
Review Patch | Details | Formatted Diff | Diff
Next iteration: updated to ToT, should address the lastest comments (16.74 KB, patch)
2008-09-24 04:06 PST, Julien Chaffraix
eric: review-
Review Patch | Details | Formatted Diff | Diff
Fixed version - Should not leak (22.08 KB, patch)
2008-11-18 15:24 PST, Julien Chaffraix
eric: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2005-11-13 10:14:35 PST
For xbl, hyatt says that we need to be able to evaluate scripts in viewless documents
------- Comment #1 From 2008-06-06 19:52:04 PST -------
Created an attachment (id=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.
------- Comment #2 From 2008-06-11 12:04:05 PST -------
(From update of attachment 21540 [details])
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.
------- Comment #3 From 2008-06-11 17:31:03 PST -------
(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().
------- Comment #4 From 2008-06-12 18:44:11 PST -------
(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.
------- Comment #5 From 2008-06-21 06:26:51 PST -------
Created an attachment (id=21870) [details]
2nd version - updated with Darin's comments
------- Comment #6 From 2008-06-23 10:39:57 PST -------
(From update of attachment 21870 [details])
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.
------- Comment #7 From 2008-06-25 06:35:38 PST -------
[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.
------- Comment #8 From 2008-06-25 16:43:26 PST -------
Created an attachment (id=21939) [details]
3nd attempt - Correct minor issues and should not leak the Page
------- Comment #9 From 2008-07-17 10:44:27 PST -------
(From update of attachment 21939 [details])
+    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.
------- Comment #10 From 2008-07-24 11:39:00 PST -------
(From update of attachment 21939 [details])
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).
------- Comment #11 From 2008-09-22 09:40:52 PST -------
Adding Eric so that he could see the forthcoming question.
------- Comment #12 From 2008-09-22 09:43:24 PST -------
(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?
------- Comment #13 From 2008-09-22 12:20:55 PST -------
(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. 
> 
------- Comment #14 From 2008-09-24 03:58:45 PST -------
(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.
------- Comment #15 From 2008-09-24 04:06:14 PST -------
Created an attachment (id=23746) [details]
Next iteration: updated to ToT, should address the lastest comments
------- Comment #16 From 2008-09-24 14:10:18 PST -------
(From update of attachment 23746 [details])
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.
------- Comment #17 From 2008-09-25 09:10:54 PST -------
(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.
------- Comment #18 From 2008-09-26 00:52:54 PST -------
> > 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.
------- Comment #19 From 2008-09-29 04:51:27 PST -------
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)
------- Comment #20 From 2008-09-30 14:05:56 PST -------
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.
------- Comment #21 From 2008-10-06 18:22:57 PST -------
(From update of attachment 23746 [details])
Marking r- since this was rolled out.
------- Comment #22 From 2008-11-18 15:24:36 PST -------
Created an attachment (id=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.
------- Comment #23 From 2008-11-18 15:29:15 PST -------
And this time I have tested it with --leaks with no abnormal increase in the number of leaks. 
------- Comment #24 From 2009-05-19 20:55:12 PST -------
(From update of attachment 25249 [details])
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.
------- Comment #25 From 2012-09-26 17:29:48 PST -------
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?