Figure out what should actually be happening with NotifyPaintEvent for non-chrome consumers

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Events
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

It looks like ever since bug 539356 our implementation of NotifyPaintEvent has returned nothing useful for any of its for non-chrome consumers: all of the actual data gets are guarded on (as of now) nsContentUtils::IsCallerChrome() calls.

Is that the expected behavior?  Can we just hide all the interface properties behind [ChromeOnly]?  Can we hide the entire interface (e.g. make it [NoInterfaceObject] or [ChromeOnly])?

It looks like we don't even fire the event to content outside of automation (which sets the "dom.send_after_paint_to_content" preference), so I doubt there's a serious web compat issue here.
Flags: needinfo?(matt.woodrow)
All the content consumers that I know of just want to know that a paint happened, they don't need any of the interface properties on the object, so making the entire interface [ChromeOnly] seems reasonable.

It looks like 'transactionId' isn't implemented as chrome-only though, so we'd need to make sure nobody has added content consumers that read it.
Flags: needinfo?(matt.woodrow)
Hmm.  Looks to me like transactionId is not read by non-chrome code in-tree.  At least assuming testing/talos/talos/tests/tabswitch/bootstrap.js and browser/base/content/tab-content.js are chrome, which I think is right at first glance.
Created attachment 8832678 [details] [diff] [review]
part 1.  Mark NotifyPaintEvent and all its members as chromeonly
Attachment #8832678 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8832679 [details] [diff] [review]
part 2.  Remove all the accessors from the XPCOM interface for NotifyPaintEvent
Attachment #8832679 - Flags: review?(bugs)
Created attachment 8832680 [details] [diff] [review]
part 3.  Remove the use of IsCallerChrome() in NotifyPaintEvent
Attachment #8832680 - Flags: review?(bugs)
Comment on attachment 8832678 [details] [diff] [review]
part 1.  Mark NotifyPaintEvent and all its members as chromeonly


> // IMPORTANT: Do not change this list without review from a DOM peer!
>     "NodeIterator",
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     "NodeList",
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     "Notification",
> // IMPORTANT: Do not change this list without review from a DOM peer!
>-    "NotifyPaintEvent",
Huh, how did this happen o_O


>+[ChromeOnly]
> interface NotifyPaintEvent : Event
> {
>+  [ChromeOnly]
>   readonly attribute DOMRectList clientRects;
> 
>+  [ChromeOnly]
>   readonly attribute DOMRect boundingClientRect;
> 
>+  [ChromeOnly]
>   readonly attribute PaintRequestList paintRequests;
> 
>+  [ChromeOnly]
>   readonly attribute unsigned long long transactionId;
> 
>+  [ChromeOnly]
>   readonly attribute DOMHighResTimeStamp paintTimeStamp;
> };
Hmm, we shouldn't dispatch the event to non-chrome at all, so ChromeOnly in the interface should be enough.
http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/layout/base/nsPresContext.cpp#2308-2312
With that, r+.
Attachment #8832678 - Flags: review?(bugs) → review+
And http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/modules/libpref/init/all.js#1161
Attachment #8832679 - Flags: review?(bugs) → review+
Comment on attachment 8832680 [details] [diff] [review]
part 3.  Remove the use of IsCallerChrome() in NotifyPaintEvent

So I think we could just remove all the nsContentUtils::IsCallerChrome() callers and that's it.
Or is there some reason why we need [ChromeOnly, NeedsCallerType]?
Attachment #8832680 - Flags: review?(bugs) → review-
> Huh, how did this happen o_O

It was exposed before we created the test.  It's there in the initial commit from bug 766694.

> Hmm, we shouldn't dispatch the event to non-chrome at all

Well, unless the pref is toggled.  I figured I'd keep things safe even in that case; I don't think we have any consumers relying on accessing the event from non-system script even when the pref is flipped (though they may want it on the non-system event _target_).

> Or is there some reason why we need [ChromeOnly, NeedsCallerType]?

I'd generally like to move all [ChromeOnly] things to the [ChromeOnly, NeedsCallerType] behavior so it's clear on the C++ side that we expect them to only be called from system code.  So if we make these things [ChromeOnly] at all, we should do the [ChromeOnly, NeedsCallerType] thing, in my opinion...
Flags: needinfo?(bugs)
OK, so I have my try results back and there are some failures.  That's because some of our non-chrome mochitests use the machinery in testing/mochitest/tests/SimpleTest/paint_listener.js, which allows waiting for paints and provides a callback with the accumulated paint rect, etc.

The computation of the accumulated paint rect is broken without this patch (always ends up with (0, 0, 0, 0)), but with the patch the computation throws an exception, breaking the tests (which presumably don't care about the rect).

I could change the test file to handle non-exposed rect as empty rect, or I guess I could just remove the [ChromeOnly] on all the members, if we're very very sure we never end up firing these things to web content, except when that test pref is set.
Flags: needinfo?(matt.woodrow)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)

> > Hmm, we shouldn't dispatch the event to non-chrome at all
> 
> Well, unless the pref is toggled.
Sure, but the pref is for testing. 

 
> I'd generally like to move all [ChromeOnly] things to the [ChromeOnly,
> NeedsCallerType] behavior so it's clear on the C++ side that we expect them
> to only be called from system code. 
Oh, I see. I like that. 

Though, in this case I think the whole interface  should just be ChromeOnly or testing-only, like it is now.
I think http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/layout/base/nsPresContext.cpp#2308-2312 is pretty clear.
and http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/layout/base/nsPresContext.cpp#2165-2169

Perhaps http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/modules/libpref/init/all.js#1161 could have a comment that one shouldn't set the pref to true except in tests.
Flags: needinfo?(bugs)
OK, so per IRC conversation the plan here is:

1)  Just make the interface itself [ChromeOnly], not its members.
2)  Rename the pref to make it clearer that it introduces a security problem.
3)  Just remove the IsCallerChrome() checks.
Created attachment 8832858 [details] [diff] [review]
part 1.  Mark the NotifyPaintEvent interface as [ChromeOnly]
Attachment #8832858 - Flags: review?(bugs)
Attachment #8832678 - Attachment is obsolete: true
Attachment #8832858 - Flags: review?(bugs) → review+
Also per IRC discussion, back to the original plan, because we have addons that actually flip the pref (e.g. to show paint events on a timeline view of what's going on with the page).
Created attachment 8832865 [details] [diff] [review]
Addendum to part 1 that fixes tests that relied on the old behavior
Attachment #8832865 - Flags: review?(bugs)
Attachment #8832680 - Flags: review- → review?(bugs)
Attachment #8832858 - Attachment is obsolete: true
Attachment #8832678 - Attachment is obsolete: false
Attachment #8832680 - Flags: review?(bugs) → review+
Comment on attachment 8832865 [details] [diff] [review]
Addendum to part 1 that fixes tests that relied on the old behavior

at least the addons should be easy to update to follow this mode.
Attachment #8832865 - Flags: review?(bugs) → review+
> at least the addons should be easy to update to follow this mode.

Indeed.  Assuming they need any changes at all.  They might not.

Comment 18

9 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edf71e10f276
part 1.  Mark NotifyPaintEvent and all its members as chromeonly.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6dae85a08ee
part 2.  Remove all the accessors from the XPCOM interface for NotifyPaintEvent.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6aaeb65203c
part 3.  Remove the use of IsCallerChrome() in NotifyPaintEvent.  r=smaug
Flags: needinfo?(matt.woodrow)

Comment 19

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edf71e10f276
https://hg.mozilla.org/mozilla-central/rev/e6dae85a08ee
https://hg.mozilla.org/mozilla-central/rev/d6aaeb65203c
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.