Closed Bug 864335 Opened 7 years ago Closed 7 years ago

Cleaning up the relationship between the document and its owner

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch removing GetScriptGlobalObject (obsolete) — Splinter Review
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...
Attachment #747955 - Flags: feedback?(bzbarsky)
Comment on attachment 747955 [details] [diff] [review]
removing GetScriptGlobalObject

Going to punt this to Blake.
Attachment #747955 - Flags: feedback?(bzbarsky) → feedback?(mrbkap)
Sorry for the delay here. I'll make sure to get to this request next week.
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?
Attachment #747955 - Flags: feedback?(mrbkap) → feedback+
(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.
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?
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.
(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!
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.
(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...
(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.
Attachment #747955 - Attachment is obsolete: true
Attachment #760799 - Flags: review?(mrbkap)
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"
Attachment #760799 - Flags: review?(mrbkap) → review+
(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
Assignee: nobody → gkrizsanits
https://hg.mozilla.org/mozilla-central/rev/544f20b3e767
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Duplicate of this bug: 431767
You need to log in before you can comment on or make changes to this bug.