videocontrols seems to listen events in default group when it probably should use system group

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: smaug, Assigned: xidorn)

Tracking

(Depends on: 2 bugs, {dev-doc-complete})

unspecified
mozilla49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
...otherwise web content calling .stopPropagation() during capture phase
prevents listeners to be called.
If system group was used, one would need to explicitly check if preventDefault() was called and depending on whether to care about that do whatever the listener is doing.
(Assignee)

Comment 1

3 years ago
The issue is videocontrols also just has content permission, so it probably cannot listen on system group. And making it able to access chrome may have security risk...
(Reporter)

Comment 2

3 years ago
JS in XBL runs with its own privileges. I don't recall what all it can do.

We could add some addSystemGroupListener to EventTarget with [Func="IsChromeOrXBL"] annontation if there is no other way to add such listener now.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 3

3 years ago
(In reply to Olli Pettay [:smaug] from comment #2)
> We could add some addSystemGroupListener to EventTarget with
> [Func="IsChromeOrXBL"] annontation if there is no other way to add such
> listener now.

That sounds like a good idea.
(Assignee)

Updated

3 years ago
Blocks: 1274146

Comment 4

3 years ago
(In reply to Olli Pettay [:smaug] from comment #0)
> ...otherwise web content calling .stopPropagation() during capture phase
> prevents listeners to be called.

Why is this a problem? Web content controls whether it wants video controls (with the controls attribute). If it asks for controls and then breaks clicking on them, that's its own fault... if it replaces the behaviour with its own behaviour, this might actually make sense (see also e.g. bug 1181055).
Component: XUL Widgets → Video/Audio Controls

Comment 5

3 years ago
(I'm kind of hoping the desired behaviour here is specced, but I'm not too optimistic that it really is...)
(Assignee)

Comment 6

3 years ago
But how the videocontrols works is a implementation detail, which shouldn't be exposed to content badly. It should not be broken unexpectedly by the content.

And we need that for fullscreenchange event anyway.

Comment 7

3 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> But how the videocontrols works is a implementation detail, which shouldn't
> be exposed to content badly. It should not be broken unexpectedly by the
> content.

Why not? Same thing's possible with e.g. submit buttons on forms, you can stop people typing in <input> fields using JS, etc. etc.

Please articulate exactly what problem you're trying to solve here. Handwavy "don't get broken by content" arguments are not very convincing. What would content do that they expect not to break video controls that does break video controls, right now? And how would we fix that, but continue to honour the case where web developers add click event handlers to the <video> element and use it to toggle play/pause (which is pretty common - e.g. youtube does things like this!).

> And we need that for fullscreenchange event anyway.

We only need it for the fullscreenchange event because of the duplicate mozfullscreenchange/fullscreenchange malarkey. And that's fine, we can change how we deal with fullscreenchange. I'm just not convinced that all the other listeners need changing.
(Reporter)

Comment 8

3 years ago
stopPropagation shouldn't affect to default handling. preventDefault() is the tool for that.

Comment 9

3 years ago
(In reply to Olli Pettay [:smaug] from comment #8)
> stopPropagation shouldn't affect to default handling. preventDefault() is
> the tool for that.

In practice, people don't do that, though. See e.g. bug 1181055.
(Assignee)

Comment 10

3 years ago
(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > stopPropagation shouldn't affect to default handling. preventDefault() is
> > the tool for that.
> 
> In practice, people don't do that, though. See e.g. bug 1181055.

Well, that fix itself makes sense but the description is... errr... The play button is supposed to act as what it looks like, so if it shows a play, it should execute play rather than "toggling the play state". This is unrelated to the topic here, I believe.
(In reply to Olli Pettay [:smaug] from comment #2)
> JS in XBL runs with its own privileges. I don't recall what all it can do.
> 
> We could add some addSystemGroupListener to EventTarget with
> [Func="IsChromeOrXBL"] annontation if there is no other way to add such
> listener now.

It's basically a content principal (technically ExpandedPrincipal([contentPrincipal]), with the additional APIs that we expose via things like IsChromeOrXBL.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 12

3 years ago
I guess I can do that directly via EventListenerOptions... So that we don't need additional methods.
(Assignee)

Comment 15

3 years ago
I actually guess we want to listen video events in the system group as well, and do not allow them to be default prevented since we just use them to update the UI.
(Assignee)

Updated

3 years ago
Assignee: nobody → bugzilla
(Reporter)

Updated

3 years ago
Attachment #8755251 - Flags: review?(bugs) → review+
(Reporter)

Comment 18

3 years ago
Comment on attachment 8755251 [details]
MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug

https://reviewboard.mozilla.org/r/54460/#review51162

This is quite nice approach indeed.

::: dom/events/EventListenerManager.cpp:1337
(Diff revision 1)
>    mTarget = nullptr;
>    RemoveAllListeners();
>  }
>  
> +static EventListenerFlags
> +GetEventListenerFlagsFromOptions(const EventListenerOptions& aOptions)

Odd that this takes just some flag from the options.
Could you rename this to GetIsSystemGroupFromOptions and only take that flag
(Assignee)

Comment 19

3 years ago
https://reviewboard.mozilla.org/r/54460/#review51162

> Odd that this takes just some flag from the options.
> Could you rename this to GetIsSystemGroupFromOptions and only take that flag

It takes all flags from EventListenerOptions, AddEventListenerOptions have some extra flags, though.

AddEventListenerOptionsOrBoolean is not convertible to EventListenerOptionsOrBoolean, so including the aOptions.IsBoolean() branch in that function is not possible in terms of code reuse.
(Reporter)

Comment 20

3 years ago
Oh, I see. Fine.

Comment 21

3 years ago
Comment on attachment 8755261 [details]
MozReview Request: Bug 1274520 part 3 - Remove unused code in touch controls. r?gijs

https://reviewboard.mozilla.org/r/54488/#review51174

If this code is dead anyway, it doesn't actually interfere with what we're doing, but it might mean the touch bindings have issues when the XBL binding gets rebound. Please instead file a followup to look at that.
Attachment #8755261 - Flags: review?(gijskruitbosch+bugs)

Comment 22

3 years ago
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

https://reviewboard.mozilla.org/r/54462/#review51182

::: toolkit/content/widgets/videocontrols.xml:1516
(Diff revision 1)
> -                      let boundFunc = func.bind(self);
> +                      let boundFunc = function(event) {
> +                        if (!event.defaultPrevented)
> +                          func.call(self, event);
> +                      };

Nit: 'boundFunc' is now inaccurate. Name it something sensible.

More generally, this changes behaviour in a way opposite to what was suggested in comment #0 in that, if our aim is to make it harder for content to 'interfere' with the video controls, we now 'obey' `defaultPrevented` on a lot more things than we used to. So for instance, you're breaking using the keyboard to interact with the video controls in this testcase:

http://jsbin.com/suziketagu/1/edit?html,js,output

While I understand that this is how web pages are "supposed" to be able to do this, I would fully expect a change like this to break web pages in the wild. And, for what it's worth, if you focus the play button (you can't see that happening, but it does...) in Chrome, on that testcase, and hit space on the keyboard, it still works, whereas your change is breaking similar behaviour when the <video> is focused in Firefox (you can't focus individual controls).

Can we be more conservative?
Attachment #8755252 - Flags: review?(gijskruitbosch+bugs)

Comment 23

3 years ago
Comment on attachment 8755262 [details]
MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs

https://reviewboard.mozilla.org/r/54490/#review51176

::: toolkit/content/widgets/videocontrols.xml:1513
(Diff revision 1)
>                      // Use the handleEvent() callback for all media events.
> -                    // The "error" event listener must capture, so that it can trap error events
> -                    // from the <source> children, which don't bubble.
> -                    for (let event of this.videoEvents)
> -                        this.video.addEventListener(event, this, (event == "error") ? true : false);
> +                    // Actually only "error" event listener needs capture, so that it can trap
> +                    // error events from <source> children, which don't bubble.
> +                    for (let event of this.videoEvents) {
> +                        this.video.addEventListener(event, this, {
> +                            capture: true,

Why change all the listeners to be capturing listeners?
Attachment #8755262 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 24

3 years ago
https://reviewboard.mozilla.org/r/54490/#review51176

> Why change all the listeners to be capturing listeners?

Because it doesn't actually matter whether they are capturing or not, and making all of them capture makes code simpler as we don't need to distinguish between error and other events.
(Assignee)

Updated

3 years ago
Attachment #8755262 - Flags: review?(gijskruitbosch+bugs)

Comment 25

3 years ago
Comment on attachment 8755262 [details]
MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs

https://reviewboard.mozilla.org/r/54490/#review51186

::: toolkit/content/widgets/videocontrols.xml:1509
(Diff revision 1)
> -                    // The "error" event listener must capture, so that it can trap error events
> -                    // from the <source> children, which don't bubble.
> +                    // Actually only "error" event listener needs capture, so that it can trap
> +                    // error events from <source> children, which don't bubble.

"Actually" is superfluous here, and '"error" event listener" needs an article (ie "the") in front of it. This doesn't explain why all the other ones are now `capture: true`. How about:

Only the "error" event listener must capture, so that it can trap error events from <source> children, which don't bubble - but we use capture for all events in order to simplify the event listener addition/removal.

TBH, I think just having the ternary for capture repeated twice would be simpler than having all the listeners capture and then having to explain that in a comment, but maybe that's just me. :-\
Attachment #8755262 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 26

3 years ago
Posted file MozReview Request: mybase (obsolete) —
Review commit: https://reviewboard.mozilla.org/r/54720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54720/
Attachment #8755251 - Attachment description: MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r?smaug → MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug
Attachment #8755252 - Attachment description: MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group, and detect defaultPrevented for them. r?gijs → MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r?gijs
Attachment #8755262 - Attachment description: MozReview Request: Bug 1274520 part 4 - Listen video events in system group. r?gijs → MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs
Attachment #8755252 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8755251 [details]
MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54460/diff/1-2/
(Assignee)

Comment 28

3 years ago
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54462/diff/1-2/
(Assignee)

Comment 29

3 years ago
Comment on attachment 8755262 [details]
MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54490/diff/1-2/
(Assignee)

Updated

3 years ago
Attachment #8755261 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8755658 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Oops, mybase is accdentally pushed... but how can that happen...

Comment 31

3 years ago
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

https://reviewboard.mozilla.org/r/54462/#review51442
Attachment #8755252 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 32

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/4f15271e5488bb0ce3c6f64179702fc03dc78582
Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug

https://hg.mozilla.org/integration/fx-team/rev/6c204be833d119fa6daae408f757e6519a531046
Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/788365ddaf61746bc66479335ae72d3e566c38ec
Bug 1274520 part 3 - Listen video events in system group. r=gijs
(Assignee)

Comment 34

3 years ago
The first patch adds a new option to EventTarget.addEventListener / removeEventListener, which needs document, though it is chrome and XBL only.
Keywords: dev-doc-needed
(Assignee)

Comment 36

3 years ago
Comment on attachment 8755251 [details]
MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54460/diff/2-3/
Attachment #8755262 - Attachment description: MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs → MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r?gijs
(Assignee)

Comment 37

3 years ago
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54462/diff/2-3/
(Assignee)

Comment 38

3 years ago
Comment on attachment 8755262 [details]
MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54490/diff/2-3/
(Assignee)

Updated

3 years ago
Attachment #8755252 - Flags: review+ → review?(gijskruitbosch+bugs)
(Assignee)

Updated

3 years ago
Attachment #8755262 - Flags: review+ → review?(gijskruitbosch+bugs)

Comment 39

3 years ago
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

https://reviewboard.mozilla.org/r/54462/#review52130

::: toolkit/content/widgets/videocontrols.xml:1042
(Diff revision 3)
> -
> -                    // Read defaultPrevented and the playback state asynchronously,
> -                    // since Web content may want to consume the "click" event
> -                    // but will only receive it after us. If web content
> +                    if (this.playButton.hasAttribute("paused")) {
> +                        this.startPlay();
> +                    } else {
> +                        this.video.pause();

This and the introduction of all the asynchronicity into the tests make me feel very uncomfortable. Some of the point of the tests is explicitly to deal with the case where web content does stuff synchronously as a result of events.

Why are these changes necessary, and can you explain how we're still testing the case(s) described in the comment you're removing here? There should probably still be a comment as to why we're reading the play button's 'paused' attribute, rather than the video state.
Attachment #8755252 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 40

3 years ago
(In reply to :Gijs Kruitbosch from comment #39)
> 
> This and the introduction of all the asynchronicity into the tests make me
> feel very uncomfortable. Some of the point of the tests is explicitly to
> deal with the case where web content does stuff synchronously as a result of
> events.

All the asynchronicity introduced in the tests are for control UI check and user input simulation. Control UI state is not detectable from the content, so that should be fine. And user inputs are asynchronously by nature.

I've realized that some of the tests are intended for testing stuff synchronously, so I carefully checked that neither DOM attribute check nor DOM method call is made asynchronous.

If the content explicitly dispatches some of the events like user inputs, it could be broken. But those kinds of actions are always fragile, and I don't believe content does that to hack the browser-provided controls rather than just providing their own in general.

> Why are these changes necessary, and can you explain how we're still testing
> the case(s) described in the comment you're removing here?

Those changes are necessary because listeners in the system group are invoked after all listeners from the content, which means at the moment a content listener is called, we haven't got the chance to update the UI.

> There should
> probably still be a comment as to why we're reading the play button's
> 'paused' attribute, rather than the video state.

Because the old way, reading the previous state and flipping it, would not work now, since the content would have changed the playing state before we getting there. So we have to do that based on the state of the button. And it also matches what I described in comment 10: the button is *not* supposed to "toggle the playing state", but to do what its appearance shows, which is controled by the attribute.

Actually the old fix in bug 1181055 is always fragile. For example, if I have "<div><video>" and I set a capture listener on div for click and call video.play() there, things would be broken again. Unless you set a capture listener on the window root , it is not possible to fill the hole completely with "toggling the playing state".

And without being able to add listener in the system group, you would always need asynchronicity to detect defaultPrevented flag.
(Assignee)

Updated

3 years ago
Attachment #8755252 - Flags: review?(gijskruitbosch+bugs)

Updated

3 years ago
Attachment #8755252 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 41

3 years ago
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

https://reviewboard.mozilla.org/r/54462/#review52148

::: toolkit/content/widgets/videocontrols.xml:1032
(Diff revision 3)
>                      }
>                      this.setFullscreenButtonState();
>                  },
>  
>                  clickToPlayClickHandler : function(e) {
> -                    if (e.button != 0)
> +                    if (e.button != 0 || e.defaultPrevented)

I suspect we should bail for `e.defaultPrevented` *after* the error check, not before.

Comment 42

3 years ago
Comment on attachment 8755262 [details]
MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs

https://reviewboard.mozilla.org/r/54490/#review52150

r=me though it might be a good idea if :jaws had a look, too.
Attachment #8755262 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

3 years ago
Attachment #8755252 - Flags: review?(jaws)
Attachment #8755262 - Flags: review?(jaws)
(Assignee)

Comment 43

3 years ago
Comment on attachment 8755251 [details]
MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54460/diff/3-4/
Attachment #8755252 - Attachment description: MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r?gijs → MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs
Attachment #8755262 - Attachment description: MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r?gijs → MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs
(Assignee)

Comment 44

3 years ago
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54462/diff/3-4/
(Assignee)

Comment 45

3 years ago
Comment on attachment 8755262 [details]
MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54490/diff/3-4/
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

https://reviewboard.mozilla.org/r/54462/#review53352

::: toolkit/content/widgets/videocontrols.xml:1042
(Diff revision 4)
> -
> +                    if (e.defaultPrevented)
> -                    // Read defaultPrevented and the playback state asynchronously,
> -                    // since Web content may want to consume the "click" event
> -                    // but will only receive it after us. If web content
> -                    // doesn't use preventDefault but still toggles playback,
> -                    // we will treat that act the same as a call to preventDefault()
> -                    // so the web content-initiated toggle is not reverted.
> -                    let previousState = this.video.paused;
> -                    setTimeout(() => {
> -                        if (e.defaultPrevented ||
> -                            this.video.paused != previousState) {
> -                            return;
> +                        return;
> +                    if (this.playButton.hasAttribute("paused")) {
> +                        this.startPlay();
> +                    } else {
> +                        this.video.pause();

How does this not regress bug 1181055? Specifically, if content doesn't use preventDefault(), what's to stop us from pausing the video that content just requested to play?
Attachment #8755252 - Flags: review?(jaws)
(Assignee)

Comment 48

3 years ago
https://reviewboard.mozilla.org/r/54462/#review53352

> How does this not regress bug 1181055? Specifically, if content doesn't use preventDefault(), what's to stop us from pausing the video that content just requested to play?

We do not pause the video at all now, because it is now execute play or pause according to the button state rather than simply toggling the playing state. The button state is updated by video play event, which is dispatched asynchronously after the click/command event, so when we are here, the state of the button hasn't been changed since the click. And the fact that this doesn't regress the test you added in bug 1181055 is an evidence that this mechanism works as expected.
(Assignee)

Updated

3 years ago
Attachment #8755252 - Flags: review?(jaws)
Comment on attachment 8755252 [details]
MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs

https://reviewboard.mozilla.org/r/54462/#review53416

Okay, thanks for the explanation.
Attachment #8755252 - Flags: review?(jaws) → review+
(Assignee)

Comment 50

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4a3ad34c694dce33e7c12fdb8e7ff681e93dae
Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/615ccf4a8bdc8cc2af1c4a16c1f89f930b0427b0
Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs,jaws

https://hg.mozilla.org/integration/mozilla-inbound/rev/2863702401b0927e2884c2b15476790265c1ff5c
Bug 1274520 part 3 - Listen video events in system group. r=gijs,jaws

Comment 51

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e4a3ad34c69
https://hg.mozilla.org/mozilla-central/rev/615ccf4a8bdc
https://hg.mozilla.org/mozilla-central/rev/2863702401b0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I'm trying to understand part 1 of this patch.  If aOptions.mMozSystemGroup, we call nsContentUtils::GetCurrentJSContext, which is mainthread-only (and asserts thus).  But EventListenerManager::AddEventListener is not mainthread-only.  What happens if the mMozSystemGroup thing is passed on a worker thread?

Anyway, it seems to me like the right thing here is to land the patches in bug 1141916, extend them slightly to support [Func="IsChromeOrXBL"] (well, not that one, that one is mainthread-only, but something similar that does the right thing on workers) on dictionary members, and not make this property web-visible at all.  As checked in, content script will see us getting it....
Flags: needinfo?(bugzilla)
(Assignee)

Comment 53

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #52)
> I'm trying to understand part 1 of this patch.  If aOptions.mMozSystemGroup,
> we call nsContentUtils::GetCurrentJSContext, which is mainthread-only (and
> asserts thus).  But EventListenerManager::AddEventListener is not
> mainthread-only.  What happens if the mMozSystemGroup thing is passed on a
> worker thread?

Hmmm, probably it would assert with debug build, and I have no idea what would happen for release build :|

> 
> Anyway, it seems to me like the right thing here is to land the patches in
> bug 1141916, extend them slightly to support [Func="IsChromeOrXBL"] (well,
> not that one, that one is mainthread-only, but something similar that does
> the right thing on workers) on dictionary members, and not make this
> property web-visible at all.  As checked in, content script will see us
> getting it....

Yeah, well, probably. I guess the right thing to do immediately here is probably to check IsMainThread explicitly before calling GetCurrentJSContext.
Flags: needinfo?(bugzilla)
(Reporter)

Comment 54

3 years ago
oh crap, I missed that.
We should make the API work consistently, so keep the existing stuff for mainthread and then explicitly check if we're in a chrome worker and only in that case allow use of mMozSystemGroup
(Assignee)

Comment 55

3 years ago
So XBL script cannot be in a worker thread currently?
(Reporter)

Comment 56

3 years ago
XBL won't ever be run in workers, but it is also that IsChromeOrXBL method is main thread only.
So need to explicitly check for chrome workers.
(Reporter)

Comment 57

3 years ago
mozilla::dom::workers::IsCurrentThreadRunningChromeWorker()
Yeah, I guess you basically want something like nsContentUtils::ThreadsafeIsCallerChrome but the mainthread branch checks IsChromeOrXBL.  And again, I would really prefer it if we did this right with conditional annotations on dictionary members.
(Assignee)

Comment 59

3 years ago
Attachment #8758937 - Flags: review?(bugs)
(Reporter)

Comment 60

3 years ago
Comment on attachment 8758937 [details] [diff] [review]
followup patch - make the option thread-safe

> static EventListenerFlags
> GetEventListenerFlagsFromOptions(const EventListenerOptions& aOptions)
Make this a method of EventListenerManager

> {
>   EventListenerFlags flags;
>   flags.mCapture = aOptions.mCapture;
>   if (aOptions.mMozSystemGroup) {
>-    JSContext* cx = nsContentUtils::GetCurrentJSContext();
>-    MOZ_ASSERT(cx, "Not being called from JS?");
>-    flags.mInSystemGroup = IsChromeOrXBL(cx, nullptr);
>+    if (NS_IsMainThread()) {
...then you can use mIsMainThreadELM here (which is faster).

>+      flags.mInSystemGroup = nsContentUtils::IsCallerChrome() ||
>+                             nsContentUtils::IsCallerContentXBL();

So this has different behavior than IsChromeOrXBL, both for IsCallerChrome part and for IsCallerContentXBL part.
I think I'd prefer to keep the existing setup for main thread: get cx and pass to IsChromeOrXBL
Attachment #8758937 - Flags: review?(bugs) → review+
(Reporter)

Comment 61

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #58)
> And again, I would really prefer it if we did this right
> with conditional annotations on dictionary members.
Sure, but need not to block work on getting webidl support for that.
(Assignee)

Comment 62

3 years ago
Making that function a method of EventListenerManager makes the line too long, and would need additional change to the header file, which isn't that necessary... So I decide to use a parameter for main thread check instead.
Attachment #8758937 - Attachment is obsolete: true
Attachment #8759128 - Flags: review?(bugs)
(Reporter)

Comment 63

3 years ago
Comment on attachment 8759128 [details] [diff] [review]
followup patch - make the option thread-safe

ok ,fine. A bit silly to pass aIsMainThread this way, but fine :)
Attachment #8759128 - Flags: review?(bugs) → review+
(Assignee)

Comment 64

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/00b8895ad68cf172ca74cecd822a2203b5d2e0a7
Bug 1274520 followup - Make EventListenerOptions.mozSystemGroup option thread-safe. r=smaug

Comment 65

3 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00b8895ad68c
followup - Make EventListenerOptions.mozSystemGroup option thread-safe. r=smaug
> Sure, but need not to block work on getting webidl support for that.

That's OK, but I just want to make sure the right solution actually happens.  Ideally before we ship this to the world...

If you want me to just do it, please let me know and I will.

(As a separate note, GetCurrentJSContext is generally a code smell nowadays, because any uses of it are preventing us from removing the JSContext stack.)
(Assignee)

Updated

3 years ago
Blocks: 1279991
(Assignee)

Comment 69

3 years ago
(In reply to Jean-Yves Perrier [:teoli] from comment #68)
> Updated:
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/
> removeEventListener#Parameters
> and
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/
> addEventListener#Parameters
> 
> Added a note in:
> https://developer.mozilla.org/en-US/Firefox/Releases/49#Interfaces

The document isn't really correct. The option is mozSystemGroup, not mozsystemgroup. I'm not sure whether it is case-sensitive, though, I suppose it is better to just match what is written in the idl file.
Fixed capitalization, thx!

Updated

2 years ago
Depends on: 1328024

Updated

2 years ago
Depends on: 1349404
You need to log in before you can comment on or make changes to this bug.