864335 - Cleaning up the relationship between the document and its owner

Status: RESOLVED FIXED

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

Description

[Gabor Krizsanits (INACTIVE)]

The more I learn about this the messier it gets. We still have mScopeObject, mWindow, mScriptGlobalObject, and more than half a dozen getters (GetScopeObject, GetScriptObject, GetScriptHandlingObject, GetInnerWindow, ...) which all returns the same object or null based on some flags, and a bunch of setters too. In top of all this GetScriptGlobalObject does that scary thing with the docshell where it all of a sudden starts returning the outer window instead of the inner... I would like to start there...

In good case scenario I can find some way to make this part a bit more sane.

In bad case scenario I understand every bit of why we need all this complexity and document the code, so it will be at least understandable what the hell is going on. I'm not sure how much time I will have for this, but would love to see any improvement in this area so I wanted to have a bug for it at least.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Attachment: removing GetScriptGlobalObject

GetScriptGlobalObject is confusing, since it sometimes returns the inner, sometimes the outer window (based on if the document is removed from the docshell or not), and there seems to be a general consensus that is should go.

Most consumers need it for getting a script context from it, then the 'native context', so the cx that belongs to the document's window. Either to push it to stack (will go...) or to do some security check / check some flags on it if the script can be run or not (should go to the docshell or to the outer window). And sometimes the JSGlobal object of the document is needed. There is a special case in HTMLCanvasElement, that I'm afraid expect fast execution and an extra QI might be a bad idea. Also it's not clear to me if it's a good idea to return the outer window there (what happens if the document is not the active document any more?). I think there we might need to introduce a FastGetGlobalJSObject on the document, or something, that part probably needs some more work...

By the way, this looks green on try, which I know, might not mean much...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 747955 [details] [diff] [review]
removing GetScriptGlobalObject

Going to punt this to Blake.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Sorry for the delay here. I'll make sure to get to this request next week.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 747955 [details] [diff] [review]
removing GetScriptGlobalObject

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

I think this approach looks very feasible. I haven't checked all of the callers yet but the fact that we're getting rid of an API that returns either the inner or the outer based on what is basically random chance is most certainly the direction we want to be heading in here.

::: content/base/src/nsDocument.cpp
@@ +4241,5 @@
> +      // The docshell returns the outer window we are done.
> +      win = do_GetInterface(requestor);
> +    }
> +  } else {
> +    win = do_GetInterface(mScriptGlobalObject);

Is this supposed to be do_GetInterface or do_QueryInterface?

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +938,5 @@
>     * invalidating a canvas will feed into heuristics and cause JIT code to be
>     * kept around longer, for smoother animations.
>     */
> +  // XXX: GetScope is probably too slow here, something better should be done here.
> +  if (nsIGlobalObject *global = OwnerDoc()->GetScopeObject()) {

Is the difference between the two functions measurable? Can you write a microbenchmark showing how much slower the extra QI is here?

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #4)
> ::: content/base/src/nsDocument.cpp
> @@ +4241,5 @@
> > +      // The docshell returns the outer window we are done.
> > +      win = do_GetInterface(requestor);
> > +    }
> > +  } else {
> > +    win = do_GetInterface(mScriptGlobalObject);
> 
> Is this supposed to be do_GetInterface or do_QueryInterface?

Good catch, the 2nd one with the mScriptGlobalObject should be definitely a do_QueryInterface.

> 
> ::: content/html/content/src/HTMLCanvasElement.cpp
> @@ +938,5 @@
> >     * invalidating a canvas will feed into heuristics and cause JIT code to be
> >     * kept around longer, for smoother animations.
> >     */
> > +  // XXX: GetScope is probably too slow here, something better should be done here.
> > +  if (nsIGlobalObject *global = OwnerDoc()->GetScopeObject()) {
> 
> Is the difference between the two functions measurable? Can you write a
> microbenchmark showing how much slower the extra QI is here?

Actually thinking this part over again, GetScopeObject always returns the inner here, which might not be the wanted behavior here... Also GetScopeObject does a weak ref to strong ref which even slower than the QI. So I probably should do a GetWindow (outher) here and then a QI. If it's measurably slower? I can try to write a benchmark... Let me ask the owner of this code first.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Hi Brian, I CC-ed you because I came across the code you landed in bug 753630 in nsHTMLCanvasElement.cpp (http://hg.mozilla.org/mozilla-central/diff/e8c20d4ba61a/content/html/content/src/nsHTMLCanvasElement.cpp) And was wondering if you are aware of the fact that GetScriptGlobalObject of document unreliably returns sometimes the inner sometimes to outer window (based on if the doc was removed from the docshell or not). 

I want to remove this function from the document, so my question is a., do you think you need inner or outer window here (so in case of a navigation away ocures the old or the new global object) and b., how performance critical is this code? Do you think an extra QI would be a big hit or not?

Comment 7

[Brian Hackett [Laid off!]]

This call should be getting the global object for the inner window.  I think a QI would be ok here, this code is called whenever a canvas is invalidated due to e.g. animation but there is already throttling to make sure that notification doesn't happen excessively often.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Brian Hackett (:bhackett) from comment #7)
> This call should be getting the global object for the inner window.  I think
> a QI would be ok here, this code is called whenever a canvas is invalidated
> due to e.g. animation but there is already throttling to make sure that
> notification doesn't happen excessively often.

Awesome, thanks!

Comment 9

[Boris Zbarsky [:bzbarsky]]

We should just fix bug 862145 and then nsPIDOMWindow for the inner will have a useful GetWrapper() that we can use without any QI silliness.

Alternately, we can add an API on nsPIDOMWindow for getting the JSObject.

In either case, I see no need to QI.

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> We should just fix bug 862145 and then nsPIDOMWindow for the inner will have
> a useful GetWrapper() that we can use without any QI silliness.

I can give it a try and see what happens if I add that wrapper cache.

> 
> Alternately, we can add an API on nsPIDOMWindow for getting the JSObject.
> 
> In either case, I see no need to QI.

That would be great. Usually this whole nsIScriptGlobalObject interface is used for 2 things, getting the js global and getting the cx... The second is most of the cases just something to fix...

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> In either case, I see no need to QI.

Problem is that adding yet another GetGlobalJSObject to nsGlobalWindow does not sound very tempting (it implements nsIScriptGlobalObject + it has a FastGetGlobalJSObject). And for the GetWrapper case, the QI is needed since GetWrapper does not belong to the nsPIDOMWindow interface what GetInnerWindow returns. Anyway, I just do a GetInnerWindow and a QI for now I think.

Comment 12

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=ec0551d6ee47

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: removing GetScriptGlobalObject. v2

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 760799 [details] [diff] [review]
removing GetScriptGlobalObject. v2

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

This makes sense to me. It'd be really good to add some comments in the .h file explaining the difference between the various getters that still exist.

::: content/base/src/nsDocument.cpp
@@ +4278,5 @@
>  nsDocument::GetWindowInternal() const
>  {
>    MOZ_ASSERT(!mWindow, "This should not be called when mWindow is not null!");
> +  // Let's use the mScriptGlobalObject. If that does not work out because
> +  // we are already removed from the docshell we can still use the docshell.

Nit: "Let's use mScriptGlobalObject". The second sentence is also not terribly clear to me.

::: content/xul/document/src/nsXULContentSink.cpp
@@ +943,5 @@
>    }
>  
>    // Don't process scripts that aren't known
>    if (langID != nsIProgrammingLanguage::UNKNOWN) {
> +      nsCOMPtr<nsIScriptGlobalObject> globalObject; // borrowed reference

Nuke the comment here?

::: dom/base/nsGlobalWindow.cpp
@@ +8060,5 @@
>      if (!doc)
>        return nullptr;             // This is ok, just means a null parent.
>  
> +    nsPIDOMWindow *pwindow = doc->GetWindow();
> +    return pwindow ? pwindow : nullptr;

This can be just "return pwindow"

Comment 15

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #14)
> This makes sense to me. It'd be really good to add some comments in the .h
> file explaining the difference between the various getters that still exist.

There are some comments in the nsIDocument.h but not very clear in some cases... I'm going to do that in the next patch. I hope I can merge some setters too

Comment 16

[Gabor Krizsanits (INACTIVE)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/544f20b3e767

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/544f20b3e767