1061864 - Add prerender flag to docshell and frameloader

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p1.patch

Comment 2

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p2.patch

Comment 3

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p3.patch

Comment 4

[(no longer active)]

Comment on attachment 8482967 [details] [diff] [review]
1061864p1.patch

Review of attachment 8482967 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +3134,5 @@
> +        if (NS_SUCCEEDED(parentAsDocShell->GetIsPrerendered(&value)))
> +        {
> +            if(value)
> +            {
> +              SetInPrerendering();

Nit: four space indentation in this file.

@@ +5775,5 @@
>  
>  NS_IMETHODIMP
> +nsDocShell::SetInPrerendering()
> +{
> +    mIsPrerendered = true;

Please MOZ_ASSERT that mIsPrerendered is false before you set it to true here to make sure people don't call SetInPrerendering by mistake on a docshell which is already in prerendering.

::: docshell/base/nsIDocShell.idl
@@ +620,5 @@
>    /**
> +   * Puts the docshell in prerendering mode
> +   */
> +  [noscript] void SetInPrerendering();
> +  readonly attribute boolean isPrerendered;

Please rev the uuid.

Also, please explain in the comment why the setter method is noscript.

Comment 5

[(no longer active)]

Comment on attachment 8482969 [details] [diff] [review]
1061864p2.patch

Review of attachment 8482969 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsIFrameLoader.idl
@@ +226,5 @@
>  
>    /**
> +   * Puts the FrameLoaderOwner in prerendering mode.
> +   */
> +  void setInPrerendering();

Please rev the uuids for both of these interfaces.

::: content/base/src/nsElementFrameLoaderOwner.cpp
@@ +112,5 @@
>    // Strangely enough, this method doesn't actually ensure that the
>    // frameloader exists.  It's more of a best-effort kind of thing.
>    mFrameLoader = nsFrameLoader::Create(thisElement, mNetworkCreated);
> +  if(mIsPrerendered)
> +  {

Nit: opening brace should go on the previous line.  Also, whitespace before ( please.

@@ +141,5 @@
>  
> +NS_IMETHODIMP
> +nsElementFrameLoaderOwner::SetInPrerendering()
> +{
> +  NS_ASSERTION(!mFrameLoader,"Please call SetInPrerendering before frameLoader is created");

Please use MOZ_ASSERT.  This assertion should be fatal.

@@ +147,5 @@
> +  if(mFrameLoader)
> +  {
> +    nsresult rv = mFrameLoader->SetInPrerendering();
> +    NS_ENSURE_SUCCESS(rv,rv);
> +  }

With the above assertion, you can remove this if block, right?

::: content/base/src/nsFrameLoader.cpp
@@ +296,5 @@
>  
> +NS_IMETHODIMP
> +nsFrameLoader::SetInPrerendering()
> +{
> +  NS_ASSERTION(!mDocShell,"Please call SetInPrerendering before docShell is created");

Nit: MOZ_ASSERT.

::: content/base/src/nsFrameLoader.h
@@ +331,5 @@
>    // enables us to detect whether the frame has moved documents during
>    // a reframe, so that we know not to restore the presentation.
>    nsCOMPtr<nsIDocument> mContainerDocWhileDetached;
>  
> +  bool mIsPrerendered = false;

Hmm, is this C++? ;-)

Please initialize in the constructor.  Also, make this a 1 bit field like the below.

::: content/xul/content/src/nsXULElement.cpp
@@ +1625,5 @@
>  nsresult
> +nsXULElement::SetInPrerendering()
> +{
> +    nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
> +    NS_ASSERTION(!frameLoader,"Please call SetInPrerendering before frameLoader is created");

MOZ_ASSERT.

@@ +1632,5 @@
> +    if(frameLoader)
> +    {
> +        nsresult rv = frameLoader->SetInPrerendering();
> +        NS_ENSURE_SUCCESS(rv,rv);
> +    }

This if block can be removed as well, right?

Comment 6

[(no longer active)]

Comment on attachment 8482971 [details] [diff] [review]
1061864p3.patch

Review of attachment 8482971 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/xul/content/test/chrome.ini
@@ +1,5 @@
>  [DEFAULT]
>  support-files =
>    398289-resource.xul
>    file_bug236853.rdf
> +  doc.html

Nit: perhaps give this a better name, such as 1061864.html.

::: content/xul/content/test/test_bug1061864_1.xul
@@ +18,5 @@
> +
> +    function RunTest()
> +    {
> +        var parentIframe = document.createElement("iframe");
> +        var frameLoaderOwner = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner);

FWIW, this QueryInterface should be no-op, so you can just remove it I think.

@@ +22,5 @@
> +        var frameLoaderOwner = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner);
> +        frameLoaderOwner.setInPrerendering();
> +        parentIframe.onload = function() {
> +            var docShell = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell;
> +            ok  (docShell.isPrerendered == 1, "this passes");

Nit:
ok(docShell.isPrerendered, "The docshell should be in prerendering mode");

::: content/xul/content/test/test_bug1061864_2.xul
@@ +23,5 @@
> +        frameLoaderOwner.setInPrerendering();
> +        parentIframe.setAttribute("src", "doc.html");
> +        parentIframe.onload = function() {
> +            var childIframe = parentIframe.contentDocument.getElementById("childiframe");
> +            console.log(childIframe);

Remove this please.

@@ +24,5 @@
> +        parentIframe.setAttribute("src", "doc.html");
> +        parentIframe.onload = function() {
> +            var childIframe = parentIframe.contentDocument.getElementById("childiframe");
> +            console.log(childIframe);
> +            var childDocShell = childIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell;

Again, the QIs should be no-op I think.

@@ +25,5 @@
> +        parentIframe.onload = function() {
> +            var childIframe = parentIframe.contentDocument.getElementById("childiframe");
> +            console.log(childIframe);
> +            var childDocShell = childIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell;
> +            ok  (childDocShell.isPrerendered == 1, "this passes");

Ditto.

::: content/xul/content/test/test_bug1061864_3.xul
@@ +22,5 @@
> +        var frameLoaderOwner = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner);
> +        parentIframe.setAttribute("src", "doc.html");
> +        parentIframe.onload = function() {
> +            var childIframe = parentIframe.contentDocument.getElementById("childiframe");
> +            console.log(childIframe);

Ditto.

@@ +24,5 @@
> +        parentIframe.onload = function() {
> +            var childIframe = parentIframe.contentDocument.getElementById("childiframe");
> +            console.log(childIframe);
> +            var childDocShell = childIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell;
> +            ok  (childDocShell.isPrerendered == 0, "this passes");

Ditto.

(Note that boolean IDL attributes map to JS true/false values.)

Comment 7

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p1.patch

Comment 8

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p2.patch

Comment 9

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p3.patch

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8484547 [details] [diff] [review]
1061864p1.patch

>+        if (NS_SUCCEEDED(parentAsDocShell->GetIsPrerendered(&value)))
>+        {
>+            if(value)
>+            {
>+                SetInPrerendering();
>+            }
>+        }
>         if (NS_FAILED(parentAsDocShell->GetAllowDNSPrefetch(&value))) {
>             value = false;
>         }
Please keep { in the same line as 'if'

>@@ -613,16 +613,23 @@ interface nsIDocShell : nsIDocShellTreeI
>   /**
>    * Sets whether a docshell is active. An active docshell is one that is
>    * visible, and thus is not a good candidate for certain optimizations
>    * like image frame discarding. Docshells are active unless told otherwise.
>    */
>   attribute boolean isActive;
> 
>   /**
>+   * Puts the docshell in prerendering mode. noscript because we want only 
>+   * native code to be able to put a docshell in prerendering.
>+   */
>+  [noscript] void SetInPrerendering();
>+  readonly attribute boolean isPrerendered;
>+
>+  /**
I don't quite understand the terminology. In C++ setter is SetInPrerendering
but getter is GetIsPrerendered.
The member variable is mIsPrerendered.

Why the difference In..ing vs. Is...ed ?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Also, what is the plan there. Will mIsPrerendered stay true once it is set to true?
But don't we want to move prerendered docshells to be visible at some, at which point I'd
assume mIsPrerendered should become false again.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8484549 [details] [diff] [review]
1061864p2.patch

>   /**
>+   * Puts the frameloader in prerendering mode.
>+   */
>+  void setInPrerendering();
Again the same terminology question.
What is the difference between
In..ing vs Is..ed
>+[scriptable, uuid(c4abebcf-55f3-47d4-af15-151311971255)]
> interface nsIFrameLoaderOwner : nsISupports
> {
>   /**
>    * The frame loader owned by this nsIFrameLoaderOwner
>    */
>   readonly attribute nsIFrameLoader frameLoader;
>   [noscript, notxpcom] alreadyAddRefed_nsFrameLoader GetFrameLoader();
> 
>   /**
>+   * Puts the FrameLoaderOwner in prerendering mode.
>+   */
>+  void setInPrerendering();
ditto

>+  if(mIsPrerendered)
>+  {
>+    mFrameLoader->SetInPrerendering();
And here again. Member variable talks about IsPrerendered, but we then call SetInPrerendering.

It is possible that the terminology issue needs just some explanation.

>   nsElementFrameLoaderOwner(mozilla::dom::FromParser aFromParser)
>     : mNetworkCreated(aFromParser == mozilla::dom::FROM_PARSER_NETWORK)
>+    , mIsPrerendered(false)
>     , mBrowserFrameListenersRegistered(false)
>     , mFrameLoaderCreationDisallowed(false)
>   {
>   }
> 
>   virtual ~nsElementFrameLoaderOwner();
> 
>   NS_DECL_NSIFRAMELOADEROWNER
>@@ -65,13 +66,14 @@ protected:
> 
>   /**
>    * True when the element is created by the parser using the
>    * NS_FROM_PARSER_NETWORK flag.
>    * If the element is modified, it may lose the flag.
>    */
>   bool mNetworkCreated;
> 
>+  bool mIsPrerendered;
So, this is fine since there aren't too many nsElementFrameLoaderOwner objects.
(html:iframes in general)
>+++ b/content/xul/content/src/nsXULElement.h
...
>+    bool mIsPrerendered;
But this is not ok. We don't want to increase memory usage of each XUL element 4 or 8 bytes.

Which iframe/browser element do we want to set prerendered btw? only xul:browser for tabs? Couldn't we just use then some
attribute as a flag?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8484550 [details] [diff] [review]
1061864p3.patch

So this might need some updates based on what happens to the other patches.

Comment 14

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Also, what is the plan there. Will mIsPrerendered stay true once it is set
> to true?

Yes.

> But don't we want to move prerendered docshells to be visible at some, at
> which point I'd
> assume mIsPrerendered should become false again.

No, that will happen by swapping the frameloaders.

Comment 15

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #12)
> >+++ b/content/xul/content/src/nsXULElement.h
> ...
> >+    bool mIsPrerendered;
> But this is not ok. We don't want to increase memory usage of each XUL
> element 4 or 8 bytes.
> 
> Which iframe/browser element do we want to set prerendered btw? only
> xul:browser for tabs?

Yes.

> Couldn't we just use then some attribute as a flag?

We could I suppose, but I would like to avoid that if possible, since the attribute would be visible on the content node...

Are you fine with making mBindingParent a tagged pointer and use its lowest bit here?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

content code can't use xul:browser.

in XULElement couldn't we move the flag to slots? We will have slots anyway in case there is
an element which may create frameloader.

Comment 17

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #16)
> content code can't use xul:browser.

I was thinking of add-ons.

> in XULElement couldn't we move the flag to slots? We will have slots anyway
> in case there is
> an element which may create frameloader.

Yes, you're right.  We should be able to put it on the slot, I think.

Comment 18

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p1.patch

Comment 19

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p2.patch

Comment 20

[Roshan Vidyashankar [:rvid]]

Attachment: 1061864p3.patch

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on Thu/Fri) from comment #14) 
> > But don't we want to move prerendered docshells to be visible at some, at
> > which point I'd
> > assume mIsPrerendered should become false again.
> 
> No, that will happen by swapping the frameloaders.

I don't understand this. Frameloader owns the docshell. What does swapping help here?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8491922 [details] [diff] [review]
1061864p1.patch

So this is ok for initial state, but
as far as I see, we'll need to have some way to clear mIsPrerendered.
Another patch for that?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8491924 [details] [diff] [review]
1061864p2.patch

>+  if (mIsPrerendered)
>+  {
{ should be in the previous line

>+  if (mIsPrerendered)
>+  {
ditto

>+nsXULElement::SetIsPrerendered()
>+{
>+    nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>+    MOZ_ASSERT(!frameLoader,"Please call SetIsPrerendered before frameLoader is created");
>+    nsXULSlots *slots= static_cast<nsXULSlots*>(Slots());
>+    if (slots) {
>+        slots->mIsPrerendered = true;
>+    }
>+    return NS_OK;
>+}
Since this method can be called from normal chrome JS, I'd prefer not having MOZ_ASSERT, but throw some exception
(INVALID_STATE or some such) in case there is frameLoader already.

>+    nsresult SetIsPrerendered();
This method could be void

>+  [ChromeOnly]
>+  void setIsPrerendered();
So this could use Throws 


...but. I think we should just some some dom attribute as a flag. This is all about chrome js/XUL. Addons can anyway see all the properties, [ChromeOnly] doesn't make it non-addon thing.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8491926 [details] [diff] [review]
1061864p3.patch

So this would need to be updated for the dom attribute approach.

The main reason I prefer dom attribute over an explicit flag is memory consumption.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Discussed with Ehsan on IRC and we do need to have a way to set a docshell non-prerendered.
Prerendered flag would be cleared during frameloader swapping.

Comment 26

[(no longer active)]

Attachment: Part 1: Add a prerendered flag to nsIDocShell

Comment 27

[(no longer active)]

Attachment: Part 2: Add a prerendered flag to nsIFrameLoader and nsIFrameLoaderOwner

Comment 28

[(no longer active)]

Attachment: Part 3: Automated tests for the prerendred flag

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8544163 [details] [diff] [review]
Part 2: Add a prerendered flag to nsIFrameLoader and nsIFrameLoaderOwner

This is for non-e10s only, but I guess that is fine for now.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ea0c21e8e33
https://hg.mozilla.org/mozilla-central/rev/75a20fb5f864
https://hg.mozilla.org/mozilla-central/rev/d07e602fd385