Closed Bug 804174 Opened 7 years ago Closed 7 years ago

Eliminate some vestigial UniversalXPConnect functions from DOM

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: mccr8, Assigned: nsm)

References

Details

Attachments

(4 files, 2 obsolete files)

With bug 789224 in place, there are at least a few places we could easily get rid of references to "UniversalXPConnect". The purpose would be to make the code more understandable.

- Eliminate nsContentUtils::CallerHasUniversalXPConnect by inlining its definition.
- Eliminate IsUniversalXPConnectCapable in nsDOMWindowUtils.cpp by inlining its definition
Assignee: nobody → nsm.nikhil
The patches remove all instances of calls, but just waiting for TBPL to confirm all builds are successful.

https://tbpl.mozilla.org/?tree=Try&rev=08254ce176db
Attachment #674876 - Flags: review?(continuation)
Comment on attachment 674876 [details] [diff] [review]
Eliminate IsUniversalXPConnectCapable

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

Looks good.
Attachment #674876 - Flags: review?(continuation) → review+
Keywords: checkin-needed
This isn't ready to check in yet. :)
Comment on attachment 674874 [details] [diff] [review]
eliminate CallerHasUniversalXPConnect

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

Looks good. I noticed a few more surrounding comments that should be updated. I'll r+ it when Bobby gets a chance to weigh in.

We should get bholley to give a thumbs up to this. We want to wait until we are sure bug 789224 is going to stick, and he may also think my idea is a bad one. :)

You should put the bug number and reviewer in the patch message for both patches to make it easier for people who check in the patch for you (I'm assuming you don't have level 3 given that you added the checkin-needed flag earlier). So, something like this:
  Bug 804164 - Eliminate CallerHasUniversalXPConnect with IsCallerChrome. r=mccr8

::: content/base/src/nsContentUtils.cpp
@@ +1605,5 @@
>      return true;
>    }
>  
>    // The subject doesn't subsume aPrincipal. Allow access only if the subject
>    // has UniversalXPConnect.

nit: Please update this comment to say "is chrome" instead of "has UniversalXPConnect" or something.

@@ +1781,5 @@
>  
>  bool
>  nsContentUtils::IsCallerTrustedForRead()
>  {
> +  return IsCallerChrome();

Hmm.  I wonder if IsCallerTrustedForRead/Write should get the same treatment.  Anyways, that can be a followup bug, if it is actually the right thing to do.

::: content/events/src/nsDOMDataTransfer.cpp
@@ -446,1 @@
>    // UniversalXPConnect privileges can always read the data. During the

nit: update the comment to say "chrome privileges" instead.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1301,2 @@
>      // setting the value of a "FILE" input widget requires the
>      // UniversalXPConnect privilege

nit: please update this comment to say "chrome privilege" instead of "the UniversalXPConnect privilege" instead.

::: dom/base/nsGlobalWindow.cpp
@@ +2600,5 @@
>  {
>    NS_ASSERTION(GetScriptableTop() &&
>                 GetScriptableTop()->GetCurrentInnerWindowInternal() == this,
>                 "DialogsAreBeingAbused called with invalid window");
>              

nit: while you are here, please remove the trailing whitespace on this blank line, and on the blank line below.

::: layout/style/nsComputedDOMStyle.cpp
@@ -537,5 @@
>                   "should not have pseudo-element data");
>    }
>  
>    // mExposeVisitedStyle is set to true only by testing APIs that
>    // require UniversalXPConnect.

nit: please change this to "chrome privilege" instead of "UniversalXPConnect" or something.
Attachment #674874 - Flags: review?(continuation)
Attachment #674874 - Flags: review?
Attachment #674874 - Flags: feedback?(bobbyholley+bmo)
Attachment #674874 - Flags: review?
Oh, and I can just check these patches in for you when the time comes.
Comment on attachment 674874 [details] [diff] [review]
eliminate CallerHasUniversalXPConnect


>   // The subject doesn't subsume aPrincipal. Allow access only if the subject
>   // has UniversalXPConnect.
>-  return CallerHasUniversalXPConnect();
>+  return IsCallerChrome();

Fix comment.


> bool
> nsContentUtils::IsCallerTrustedForRead()
> {
>-  return CallerHasUniversalXPConnect();
>+  return IsCallerChrome();
> }
> 
> bool
> nsContentUtils::IsCallerTrustedForWrite()
> {
>-  return CallerHasUniversalXPConnect();
>+  return IsCallerChrome();
> }

These functions should be eliminated, and their definitions inlined as well. I know mccr8 suggested doing this in a separate bug, but there's no time like the present.

>   // mExposeVisitedStyle is set to true only by testing APIs that
>   // require UniversalXPConnect.
>   NS_ABORT_IF_FALSE(!mExposeVisitedStyle ||
>-                    nsContentUtils::CallerHasUniversalXPConnect(),
>+                    nsContentUtils::IsCallerChrome(),
>                     "mExposeVisitedStyle set incorrectly");

Update comment.
Attachment #674874 - Flags: feedback?(bobbyholley+bmo) → feedback+
Thanks Bobby!

Nikhil, update your patch to address our comments (including inlining IsCallerTrustedForRead and IsCallerTrustedForWrite) and then I'll review it again. The patch doesn't seem very dangerous, so I wouldn't bother with a try run again: just make sure it builds locally and runs for at least long enough to open a page or two. :)
Incidentally, dom/base/nsJSEnvironment.cpp has "CheckUniversalXPConnectForTraceMalloc()", I think that should be changed eventually. Working on comments for now.

Same for:
dom/base/nsDOMWindowUtils.cpp "IsUniversalXPConnectCapable()"
sorry, "Same for" doesn't apply in the above comment, I just fixed that
Comment on attachment 675218 [details] [diff] [review]
eliminate CallerHasUniversalXPConnect

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

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1056,2 @@
>          // setting the value of a "FILE" input widget requires the
>          // UniversalXPConnect privilege

nit: this comment needs to be updated.  I can just do this before I land the patch.
Attachment #675218 - Flags: review?(continuation) → review+
Attachment #675220 - Flags: review?(continuation) → review+
Comment on attachment 675221 [details] [diff] [review]
Part 3 - Eliminate IsCallerTrustedForRead

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

Looks good.
Attachment #675221 - Flags: review?(continuation) → review+
Comment on attachment 675222 [details] [diff] [review]
Part 4 - Eliminate IsCallerTrustedForWrite

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

Looks good.  Would you like me to land this now?
Attachment #675222 - Flags: review?(continuation) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.