Closed
Bug 1334957
Opened 7 years ago
Closed 7 years ago
Figure out what should actually be happening with NotifyPaintEvent for non-chrome consumers
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
2.39 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.74 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.73 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8832678 -
Flags: review?(bugs)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8832679 -
Flags: review?(bugs)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8832680 -
Flags: review?(bugs)
Comment 6•7 years ago
|
||
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+
Comment 7•7 years ago
|
||
And http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/modules/libpref/init/all.js#1161
Updated•7 years ago
|
Attachment #8832679 -
Flags: review?(bugs) → review+
Comment 8•7 years ago
|
||
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-
Assignee | ||
Comment 9•7 years ago
|
||
> 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)
Assignee | ||
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
(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)
Assignee | ||
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8832858 -
Flags: review?(bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8832678 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8832858 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 14•7 years ago
|
||
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).
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8832865 -
Flags: review?(bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8832680 -
Flags: review- → review?(bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8832858 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8832678 -
Attachment is obsolete: false
Updated•7 years ago
|
Attachment #8832680 -
Flags: review?(bugs) → review+
Comment 16•7 years ago
|
||
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+
Assignee | ||
Comment 17•7 years ago
|
||
> 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•7 years 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
Updated•7 years ago
|
Flags: needinfo?(matt.woodrow)
Comment 19•7 years 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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•