Closed Bug 1497940 Opened 6 years ago Closed 6 years ago

Convert pluginProblem to UA Widget

Categories

(Core :: XBL, task, P2)

Product:
Core
Component:
XBL
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(5 files, 8 obsolete files)

Fix the assertion.
1.51 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part1: move AttachAndSetUAShadowRoot to generic HTML element
7.92 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Bug 1497940 - Part III, Remove unused pluginProblem mobile styles
47 bytes, text/x-phabricator-request
Details | Review
Bug 1497940 - Part IV, Move pluginProblem resources from chrome://mozapps to chrome://global
47 bytes, text/x-phabricator-request
Details | Review
Bug 1497940 - Part V, Convert pluginProblem to UA Widget
47 bytes, text/x-phabricator-request
Details | Review 
https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/toolkit/pluginproblem/content/pluginProblem.xml

We'll need to figure out:

- where in C++ to dispatch the UAWidget events (for videocontrols we do that in HTMLMediaElement::BindToTree etc.)
- where to add UA Widget Shadow Root, which replaces content. (for videocontrols we always add it when pref'd on)
- whether or not Shadow Root has to be removed in some point. (for videocontrols it's never removed)
- how to turn off XBL impl when pref'd on (for videocontrols we simply have nsVideoFrame never contruct <xul:videocontrols> which XBL binds on it)

To keep the patch minimal (and have XBL continue to function), I guess we could keep the CSS selectors. Other than that the above questions need some research.
Priority: -- → P2
Here's some findings, it'd be good to make a sanity check with someone experienced with the code.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #0)
> We'll need to figure out:
> 
> - where in C++ to dispatch the UAWidget events (for videocontrols we do that
> in HTMLMediaElement::BindToTree etc.)

It looks like plugin CSS pseudo styles [1] carried by rust stylo [2] are connected via EventState [3]. Thus it seems reasonable if  nsObjectLoadingContent::NotifyStateChanged was firing UAWidgetBindToTree/UAWidgetUnbindFromTree events, when state is set/unset to one of blocked/chrased etc.

> - where to add UA Widget Shadow Root, which replaces content. (for
> videocontrols we always add it when pref'd on)

not sure if I understand all niceties here, but could it be at the same place, where UAWidgetBindToTree is fired?

> - whether or not Shadow Root has to be removed in some point. (for
> videocontrols it's never removed)

I'd say it makes sense to remove it when UAWidgetUnbindFromTree is fired, because it is one way ticket from the user perspective - the users don't usually move back and forth from plugin and fallback content.

> - how to turn off XBL impl when pref'd on (for videocontrols we simply have
> nsVideoFrame never contruct <xul:videocontrols> which XBL binds on it)

how about not firing UAWidgetBindToTree event if it's prefed out?

[1] https://searchfox.org/mozilla-central/source/toolkit/pluginproblem/content/pluginProblemBinding.css#25
[2] https://searchfox.org/mozilla-central/source/servo/components/style/element_state.rs#63
[3] https://searchfox.org/mozilla-central/source/dom/events/EventStates.h#237
[4] https://searchfox.org/mozilla-central/source/dom/base/nsObjectLoadingContent.cpp#2674

Emilio, are you familiar with mentioned code?
Flags: needinfo?(emilio)
(In reply to alexander :surkov (:asurkov) from comment #1)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #0)
> > We'll need to figure out:
> > 
> > - where in C++ to dispatch the UAWidget events (for videocontrols we do that
> > in HTMLMediaElement::BindToTree etc.)
> 
> It looks like plugin CSS pseudo styles [1] carried by rust stylo [2] are
> connected via EventState [3]. Thus it seems reasonable if 
> nsObjectLoadingContent::NotifyStateChanged was firing
> UAWidgetBindToTree/UAWidgetUnbindFromTree events, when state is set/unset to
> one of blocked/chrased etc.
> 
> > - where to add UA Widget Shadow Root, which replaces content. (for
> > videocontrols we always add it when pref'd on)
> 
> not sure if I understand all niceties here, but could it be at the same
> place, where UAWidgetBindToTree is fired?
> 
> > - whether or not Shadow Root has to be removed in some point. (for
> > videocontrols it's never removed)

I think those points above seem reasonable. You generally want to tie the events to the ShadowRoot.

You probably want to prevent adding and removing the shadow root over and over (and note that you can only remove a ShadowRoot if there are no slots in the page). But I think the state changes for plugins shouldn't be frequent, and I don't think a <slot> would be needed here either (though maybe I'm wrong here?).

Depending on whether there are slots or not, you may want to just create the UA shadow unconditionally and switch between slotted / fallback content on state changes instead.

You need to call UnbindFromTree when the object is disconnected from the DOM, and similarly you don't probably want to load the UA widget, even if the plugin has the broken state, but it's out of the document.

So there'd be three places to check: BindToTree, UnbindFromTree and NotifyStateChanged.

> I'd say it makes sense to remove it when UAWidgetUnbindFromTree is fired,
> because it is one way ticket from the user perspective - the users don't
> usually move back and forth from plugin and fallback content.
> 
> > - how to turn off XBL impl when pref'd on (for videocontrols we simply have
> > nsVideoFrame never contruct <xul:videocontrols> which XBL binds on it)
> 
> how about not firing UAWidgetBindToTree event if it's prefed out?

It's a bit more complicated than that. You really don't want to have an element with both a binding and a Shadow Root.

So you need to prevent the load of pluginProblemBinding.css when the pref is off, or prevent that rule from getting applied... @supports -moz-bool-pref(<the-ua-widget-pref>) may be an acceptable solution if you don't care about runtime-switching between UA-widget and non-UA-widget, but otherwise it may need some work. That work could just be loading pluginProblemBinding.css where we load the rest of our UA sheets:

  https://searchfox.org/mozilla-central/rev/c56977420df7a1b692ce0f7e499ddb364d9fd7b2/layout/base/nsDocumentViewer.cpp#2496

But I have no idea why pluginProblemBinding.css is loaded via manifest. Should be easy to move it to the layout stylesheet cache, if there's no strong reason for not doing that.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
> (In reply to alexander :surkov (:asurkov) from comment #1)
> > how about not firing UAWidgetBindToTree event if it's prefed out?
> 
> It's a bit more complicated than that. You really don't want to have an
> element with both a binding and a Shadow Root.
> 
> So you need to prevent the load of pluginProblemBinding.css when the pref is
> off, or prevent that rule from getting applied... @supports
> -moz-bool-pref(<the-ua-widget-pref>) may be an acceptable solution if you
> don't care about runtime-switching between UA-widget and non-UA-widget, but
> otherwise it may need some work.

Good idea, I hadn't thought of that but it seems like the most simple solution (the pref is `dom.ua_widget.enabled`). My understanding is that we don't need to worry about runtime switching. I guess the only thing would be if we want to test both modes in the same test directory. But I really don't want to spend too much effort supporting this case if we can avoid it - it's not something that a user would typically see.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Good idea, I hadn't thought of that but it seems like the most simple
> solution (the pref is `dom.ua_widget.enabled`). My understanding is that we
> don't need to worry about runtime switching. I guess the only thing would be
> if we want to test both modes in the same test directory. But I really don't
> want to spend too much effort supporting this case if we can avoid it - it's
> not something that a user would typically see.

And also, AIUI that pref is temporary while we have to support Shadow DOM being preffed off. Once the SD pref is removed we will remove the UA widget pref (and delete the in-content binding code).
AttachAndSetUAShadowRoot should live in a base class to be reused by derived classes, the patch was stolen from bug 1496242.
Assignee: nobody → surkov.alexander
Attachment #9019224 - Flags: review?(bugs)
correct one
Attachment #9019224 - Attachment is obsolete: true
Attachment #9019224 - Flags: review?(bugs)
Attachment #9019260 - Flags: review?(bugs)
Attached patch part2 (obsolete) — — Splinter Review 
Emilio, could you please take a look at the patch to see if I captured your words right
Attachment #9019262 - Flags: feedback?(emilio)
Comment on attachment 9019262 [details] [diff] [review]
part2

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

I think nsObjectLoadingContent.cpp can be simplified, if you create the widget unconditionally on BindToTree like you're doing.

I understood (maybe wrongly) that the idea was to hook / unhook the shadow root dynamically when states change, but I think this is better. This way you don't need about ContentStateChanged at all.

::: browser/actors/PluginChild.jsm
@@ +122,5 @@
>  
>    getPluginUI(plugin, anonid) {
>      return plugin.ownerDocument.
> +      getAnonymousElementByAttribute(plugin, "anonid", anonid) ||
> +      plugin.querySelector(`[anonid="${anonid}"]`);

If plugin is the <object> element, this needs to look into the shadow root, otherwise it won't work.

::: dom/base/nsObjectLoadingContent.cpp
@@ +611,5 @@
> +  // Create a shadow DOM for plugin problem UIWidget.
> +  if (nsContentUtils::IsUAWidgetEnabled()) {
> +    nsCOMPtr<nsIContent> thisNode =
> +      do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> +    if (thisNode->IsInComposedDoc()) {

I think this check should be consistent with the AddPlugin check. But I think that check is wrong for Shadow DOM and should be changed for IsInComposedDoc()...

@@ +612,5 @@
> +  if (nsContentUtils::IsUAWidgetEnabled()) {
> +    nsCOMPtr<nsIContent> thisNode =
> +      do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> +    if (thisNode->IsInComposedDoc()) {
> +      nsGenericHTMLElement::FromNode(thisNode)->AttachAndSetUAShadowRoot();

So, if you're calling AttachAndSetUAShadowRoot() unconditionally from here (instead of just when the object gets one of the states), then you don't even need the BindToTree / UnbindFromTree stuff from ContentStateChanged, right?

@@ +647,5 @@
>      UnloadObject();
>    }
> +
> +  // Unattach plugin problem UIWidget if any.
> +  if (thisElement->GetShadowRoot()) {

This needs to check whether we were in the composed doc so you always get one unbind for each bind and not more. IsInComposedDoc() should return the right thing here still.

That looks like a problem for HTMLMediaElement right now... :(

@@ +2704,5 @@
>        nsAutoScriptBlocker scriptBlocker;
>        doc->ContentStateChanged(thisContent, changedBits);
>      }
> +
> +    // Create plugin problem UIWidget if needed.

nit: UAWidget

@@ +2712,5 @@
> +      NS_EVENT_STATE_TYPE_CLICK_TO_PLAY |
> +      NS_EVENT_STATE_VULNERABLE_UPDATABLE |
> +      NS_EVENT_STATE_VULNERABLE_NO_UPDATE;
> +
> +    if ((aOldState & pluginProblemState) != (newState & pluginProblemState) &&

Hmm, you're dispatching two events when any event changes. Do you really want both a bind and unbind event when we go from blocked to crashed for example?

If so, if (!(changedBits & pluginProblemState).IsEmpty()) may be an easier-to.

@@ +2723,5 @@
> +                                   ChromeOnlyDispatch::eYes);
> +        dispatcher->RunDOMEventWhenSafe();
> +      }
> +
> +      if (newState.HasState(pluginProblemState)) {

This will assert in debug builds:

  https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/dom/events/EventStates.h#119

Need to use HasAnyOfStates.
Attachment #9019262 - Flags: feedback?(emilio)
Comment on attachment 9019262 [details] [diff] [review]
part2

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

::: toolkit/pluginproblem/content/pluginProblem.js
@@ +3,5 @@
> +  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +// This is loaded into all XUL windows. Wrap in a block to prevent

This comment can be removed (along with the wrapping brackets around the rest of the file), and replaced with something like: https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/toolkit/content/widgets/videocontrols.js#7
Comment on attachment 9019262 [details] [diff] [review]
part2

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

::: toolkit/pluginproblem/content/pluginProblemBinding.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @namespace url(http://www.w3.org/1999/xhtml); /* set default namespace to HTML */
>  
> +@supports -moz-bool-pref("dom.ua_widget.enabled") {

This needs to be negated, like so:

```css
@supports not -moz-bool-pref("dom.ua_widget.enabled") {
    …
}
```

See also:
- https://developer.mozilla.org/docs/Mozilla/Gecko/Chrome/CSS/-moz-bool-pref
(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> Comment on attachment 9019262 [details] [diff] [review]
> part2
> 
> Review of attachment 9019262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think nsObjectLoadingContent.cpp can be simplified, if you create the
> widget unconditionally on BindToTree like you're doing.
> 
> I understood (maybe wrongly) that the idea was to hook / unhook the shadow
> root dynamically when states change, but I think this is better. This way
> you don't need about ContentStateChanged at all.

I'm not sure I captured your words properly. So whenever plugin goes into a "bad" state, then we need to show UA widget and hide explicit (fallback) content. If the plugin gets into a "good" state, then UA widget should be hidden/removed. I got to think it's good idea to create shadow content, associate it to the widget and leave it alone. Then whenever state is changed then fire UAWidgetBind/Unbind events to show/hide the widget itself. If this is not working scenario or it can be improved, then please speak up, I might be misunderstood you.

> >    getPluginUI(plugin, anonid) {
> >      return plugin.ownerDocument.
> > +      getAnonymousElementByAttribute(plugin, "anonid", anonid) ||
> > +      plugin.querySelector(`[anonid="${anonid}"]`);
> 
> If plugin is the <object> element, this needs to look into the shadow root,
> otherwise it won't work.

right, so plugin.shadowRoot.querySelector should work, correct or there some restrictions on how UA widget content is exposed?

> @@ +612,5 @@
> > +  if (nsContentUtils::IsUAWidgetEnabled()) {
> > +    nsCOMPtr<nsIContent> thisNode =
> > +      do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> > +    if (thisNode->IsInComposedDoc()) {
> > +      nsGenericHTMLElement::FromNode(thisNode)->AttachAndSetUAShadowRoot();
> 
> So, if you're calling AttachAndSetUAShadowRoot() unconditionally from here
> (instead of just when the object gets one of the states), then you don't
> even need the BindToTree / UnbindFromTree stuff from ContentStateChanged,
> right?

I'm not sure I understand UAWidget mechanics. So I assumed that the object can be unbound from the tree without state change notifications, but UAWidget has to be turned off in such case. If we don't care about shutting down UAWidget in this case, then I think I can simply remove UnbindFromTree() part.

> > +  // Unattach plugin problem UIWidget if any.
> > +  if (thisElement->GetShadowRoot()) {
> 
> This needs to check whether we were in the composed doc so you always get
> one unbind for each bind and not more. IsInComposedDoc() should return the
> right thing here still.

ok, it is tricky indeed.

> That looks like a problem for HTMLMediaElement right now... :(

should I file a bug/patch on it?

> @@ +2712,5 @@
> > +      NS_EVENT_STATE_TYPE_CLICK_TO_PLAY |
> > +      NS_EVENT_STATE_VULNERABLE_UPDATABLE |
> > +      NS_EVENT_STATE_VULNERABLE_NO_UPDATE;
> > +
> > +    if ((aOldState & pluginProblemState) != (newState & pluginProblemState) &&
> 
> Hmm, you're dispatching two events when any event changes. Do you really
> want both a bind and unbind event when we go from blocked to crashed for
> example?

I presumed that I copied XBL behavior, I assumed that whenever pseudo style is changed, then the binding is reattached, and PluginChild.jsm might have dependencies on it (I could be wrong in my assumptions though).
Flags: needinfo?(emilio)
Comment on attachment 9019262 [details] [diff] [review]
part2

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

::: toolkit/actors/UAWidgetsChild.jsm
@@ +54,5 @@
>          break;
>        case "input":
>          // TODO (datetimebox)
>          break;
>        case "applet":

And here as well I guess.

::: toolkit/pluginproblem/content/pluginProblemBinding.css
@@ +10,5 @@
>  embed:-moz-handler-crashed,
>  embed:-moz-handler-clicktoplay,
>  embed:-moz-handler-vulnerable-updatable,
>  embed:-moz-handler-vulnerable-no-update,
>  applet:-moz-handler-blocked,

If while at it you could remove all the applet stuff it'd be amazing. applet is no more (https://bugzilla.mozilla.org/show_bug.cgi?id=1279218)
(In reply to alexander :surkov (:asurkov) from comment #11)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> > Comment on attachment 9019262 [details] [diff] [review]
> > part2
> > 
> > Review of attachment 9019262 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think nsObjectLoadingContent.cpp can be simplified, if you create the
> > widget unconditionally on BindToTree like you're doing.
> > 
> > I understood (maybe wrongly) that the idea was to hook / unhook the shadow
> > root dynamically when states change, but I think this is better. This way
> > you don't need about ContentStateChanged at all.
> 
> I'm not sure I captured your words properly. So whenever plugin goes into a
> "bad" state, then we need to show UA widget and hide explicit (fallback)
> content. If the plugin gets into a "good" state, then UA widget should be
> hidden/removed. I got to think it's good idea to create shadow content,
> associate it to the widget and leave it alone. Then whenever state is
> changed then fire UAWidgetBind/Unbind events to show/hide the widget itself.
> If this is not working scenario or it can be improved, then please speak up,
> I might be misunderstood you.

This patch breaks the fallback content right? There's no <slot>, so if you attach a shadow root when the plugin should have fallback content you don't render anything.

You can either add / remove the shadow root, or use slots for this, I guess.

> > >    getPluginUI(plugin, anonid) {
> > >      return plugin.ownerDocument.
> > > +      getAnonymousElementByAttribute(plugin, "anonid", anonid) ||
> > > +      plugin.querySelector(`[anonid="${anonid}"]`);
> > 
> > If plugin is the <object> element, this needs to look into the shadow root,
> > otherwise it won't work.
> 
> right, so plugin.shadowRoot.querySelector should work, correct or there some
> restrictions on how UA widget content is exposed?

shadowRoot will return null, since it's a closed shadow tree. I believe Chrome code can use openOrClosedShadowRoot for this.

> > @@ +612,5 @@
> > > +  if (nsContentUtils::IsUAWidgetEnabled()) {
> > > +    nsCOMPtr<nsIContent> thisNode =
> > > +      do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> > > +    if (thisNode->IsInComposedDoc()) {
> > > +      nsGenericHTMLElement::FromNode(thisNode)->AttachAndSetUAShadowRoot();
> > 
> > So, if you're calling AttachAndSetUAShadowRoot() unconditionally from here
> > (instead of just when the object gets one of the states), then you don't
> > even need the BindToTree / UnbindFromTree stuff from ContentStateChanged,
> > right?
> 
> I'm not sure I understand UAWidget mechanics. So I assumed that the object
> can be unbound from the tree without state change notifications, but
> UAWidget has to be turned off in such case. If we don't care about shutting
> down UAWidget in this case, then I think I can simply remove
> UnbindFromTree() part.
> 
> > > +  // Unattach plugin problem UIWidget if any.
> > > +  if (thisElement->GetShadowRoot()) {
> > 
> > This needs to check whether we were in the composed doc so you always get
> > one unbind for each bind and not more. IsInComposedDoc() should return the
> > right thing here still.
> 
> ok, it is tricky indeed.
> 
> > That looks like a problem for HTMLMediaElement right now... :(
> 
> should I file a bug/patch on it?

Yeah, though looking at UAWidgetsChild.jsm it's not a problem I guess, because of this early-return:

  https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/toolkit/actors/UAWidgetsChild.jsm#84

I guess I'd ask Tim what's the expectation, but I think it'd be good to not dispatch useless / unbalanced events.

> > @@ +2712,5 @@
> > > +      NS_EVENT_STATE_TYPE_CLICK_TO_PLAY |
> > > +      NS_EVENT_STATE_VULNERABLE_UPDATABLE |
> > > +      NS_EVENT_STATE_VULNERABLE_NO_UPDATE;
> > > +
> > > +    if ((aOldState & pluginProblemState) != (newState & pluginProblemState) &&
> > 
> > Hmm, you're dispatching two events when any event changes. Do you really
> > want both a bind and unbind event when we go from blocked to crashed for
> > example?
> 
> I presumed that I copied XBL behavior, I assumed that whenever pseudo style
> is changed, then the binding is reattached, and PluginChild.jsm might have
> dependencies on it (I could be wrong in my assumptions though).

Hmm, that's not the case. When state changes between two states to which that rule applies, no style changes, so we literally do nothing.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

> > I'm not sure I captured your words properly. So whenever plugin goes into a
> > "bad" state, then we need to show UA widget and hide explicit (fallback)
> > content. If the plugin gets into a "good" state, then UA widget should be
> > hidden/removed. I got to think it's good idea to create shadow content,
> > associate it to the widget and leave it alone. Then whenever state is
> > changed then fire UAWidgetBind/Unbind events to show/hide the widget itself.
> > If this is not working scenario or it can be improved, then please speak up,
> > I might be misunderstood you.
> 
> This patch breaks the fallback content right? There's no <slot>, so if you
> attach a shadow root when the plugin should have fallback content you don't
> render anything.
> 
> You can either add / remove the shadow root, or use slots for this, I guess.

pluginProblem binding always keep fallback hidden as far as I can tell. Thus it seems reasonable to add/remove shadow DOM to turn on/off plugins. Btw, what is a proper way to remove shadow DOM?

> > right, so plugin.shadowRoot.querySelector should work, correct or there some
> > restrictions on how UA widget content is exposed?
> 
> shadowRoot will return null, since it's a closed shadow tree. I believe
> Chrome code can use openOrClosedShadowRoot for this.

thanks!

> > I presumed that I copied XBL behavior, I assumed that whenever pseudo style
> > is changed, then the binding is reattached, and PluginChild.jsm might have
> > dependencies on it (I could be wrong in my assumptions though).
> 
> Hmm, that's not the case. When state changes between two states to which
> that rule applies, no style changes, so we literally do nothing.

gotcha
Attached patch part2: v3 (obsolete) — — Splinter Review 

        Attachment #9019262 -
        Attachment is obsolete: true

        Attachment #9019924 -
        Flags: review?(emilio)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part2 (build, not tested yet) (obsolete)
        — 
          — Splinter Review
      

    


  
Does this one look good?

        Attachment #9019924 -
        Attachment is obsolete: true

        Attachment #9019924 -
        Flags: review?(emilio)

        Attachment #9019936 -
        Flags: review?(emilio)

    
    

    
  
Bug 1499476 added the ability to use MozXULElement features from non-XUL elements by turning it into a “pseudo‑mixin” (real mixins aren’t yet supported in JavaScript), so it might be possible to use that.
Depends on: 1499476

    
    

    
  
(In reply to ExE Boss from comment #17)
> Bug 1499476 added the ability to use MozXULElement features from non-XUL
> elements by turning it into a “pseudo‑mixin” (real mixins aren’t yet
> supported in JavaScript), so it might be possible to use that.

We don't use Custom Elements for UA widgets running in the page. I'm not sure what feature specifically we'd want from that class but we can implement it directly into the plugin problem widget if needed.
No longer depends on: 1499476

    
    

    
  
Comment on attachment 9019936 [details] [diff] [review]
part2 (build, not tested yet)

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +645,5 @@
> +                               NS_LITERAL_STRING("UAWidgetUnbindFromTree"),
> +                               CanBubble::eYes,
> +                               ChromeOnlyDispatch::eYes);
> +    dispatcher->RunDOMEventWhenSafe();
> +    thisEl->UnattachShadow();

There's no way this builds right? thisEl is not defined.

::: toolkit/pluginproblem/content/pluginProblem.js
@@ +75,5 @@
> +        </div>
> +        <button class="closeIcon" anonid="closeIcon" title="FROM-DTD.hidePluginBtn.label;"></button>
> +      </div>
> +      <div style="display:none;">
> +        <children></children>

I don't think <children> makes sense here.

@@ +88,5 @@
> +
> +  }
> +}
> +
> +customElements.define("pluginProblem", MozPluginProblem);

Hmm, which global is this custom-element defined here? If content has a <pluginProblem> element, do they get this behavior?

Also, am I missing something? Where's the pluginProblem element created?

        Attachment #9019936 -
        Flags: review?(emilio) → review-

    
    

    
  
(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)
> > +customElements.define("pluginProblem", MozPluginProblem);
> 
> Hmm, which global is this custom-element defined here? If content has a
> <pluginProblem> element, do they get this behavior?
> 
> Also, am I missing something? Where's the pluginProblem element created?

You're right - UA widgets aren't supposed to be registered via CustomElementsRegistry. Alex, see the docs at https://firefox-source-docs.mozilla.org/toolkit/content/toolkit_widgets/ua_widget.html and the videocontrols implementation (or datetimebox patches at Bug 1496242) for a reference implementation.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part2 (obsolete)
        — 
          — Splinter Review
      

    


  
this one builds, and passes few tests more than the previous one, but still there are failures.

one of the failing tests is  dom/plugins/test/mochitest/test_crash_notify.xul  , which asserts [1]:

ASSERTION: Element not in doc!: 'aContent->GetUncomposedDoc() == this

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=207954253&repo=try&lineNumber=11458

Any pointers where to look at?

        Attachment #9019936 -
        Attachment is obsolete: true

    
    

    
  
Emilio, could you help with comment 21 please?
Flags: needinfo?(emilio)

    
    

    
  
Sure, I can try.

    
    

    
  


  
We notify the document when appending to a connected shadow root, that assertion doesn't really hold.
Flags: needinfo?(emilio)

        Attachment #9020754 -
        Flags: review?(bugs)

    
    

    
  
I filed bug 1502875 on removing most of that code, which is mostly useless nowadays.

        Attachment #9020754 -
        Flags: review?(bugs) → review+

    
    

    
  
(I landed the assertion fix since it's independent)

    
    

    
  
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed31fa097a65
Fix an assertion in XULDocument::AddSubtreeToDocument. r=smaug

    
    

    
  
(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)
> (I landed the assertion fix since it's independent)

thanks a heap!

    
    

    
  
Comment on attachment 9019260 [details] [diff] [review]
part1: move AttachAndSetUAShadowRoot to generic HTML element

It would be nice to assert hard if one tries to use AttachAndSetUAShadowRoot with any of the normally allowed elements, but I guess that shouldn't happen.

        Attachment #9019260 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Olli Pettay [:smaug] (high review load) from comment #30)
> Comment on attachment 9019260 [details] [diff] [review]
> part1: move AttachAndSetUAShadowRoot to generic HTML element
> 
> It would be nice to assert hard if one tries to use AttachAndSetUAShadowRoot
> with any of the normally allowed elements, but I guess that shouldn't happen.

np, though I'm not sure how to assert about the normally allowed elements, i.e. what makes a normally allowed element, suggestions please?
Flags: needinfo?(bugs)

    
    

    
  
Yup. That list is coming from the spec and we should attach UA Widgets to such since we page may do so.
Adding that list to some helper method and using that in AttachShadow and in debug code of AttachAndSetUAShadowRoot would be nice.
Flags: needinfo?(bugs)

    
    

    
  
(In reply to Olli Pettay [:smaug] (high review load) from comment #33)
> Yup. That list is coming from the spec and we should attach UA Widgets to
> such since we page may do so.

I thinking you mean “we shouldn’t attach UA Widgets to such elements”.

    
    

    
  
(In reply to Olli Pettay [:smaug] (high review load) from comment #33)
> Yup. That list is coming from the spec and we should attach UA Widgets to
> such since we page may do so.
> Adding that list to some helper method and using that in AttachShadow and in
> debug code of AttachAndSetUAShadowRoot would be nice.

It appears that AttachAndSetUAShadowRoot is used to attach to different set of element, like videocontrols or plugin, which are not listed in AttachShadow. Are you sure they should share same method?
Flags: needinfo?(bugs)

    
    

    
  
(In reply to alexander :surkov (:asurkov) from comment #35)
> (In reply to Olli Pettay [:smaug] (high review load) from comment #33)
> > Yup. That list is coming from the spec and we should attach UA Widgets to
> > such since we page may do so.
> > Adding that list to some helper method and using that in AttachShadow and in
> > debug code of AttachAndSetUAShadowRoot would be nice.
> 
> It appears that AttachAndSetUAShadowRoot is used to attach to different set
> of element, like videocontrols or plugin, which are not listed in
> AttachShadow. Are you sure they should share same method?

ignore it
Flags: needinfo?(bugs)

    
    

    
  


  
add assertion

        Attachment #9019260 -
        Attachment is obsolete: true

        Attachment #9021746 -
        Flags: review?(bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        wip (obsolete)
        — 
          — Splinter Review
      

    


  
for some reason chrome://mozapps/skin/plugins/pluginProblem.css seems not loading (when I tweak .mainBox class then the styles are not applied), cannot understand why, chrome://pluginproblem/content/pluginProblemContent.css is loaded just fine (tweaking .mainBox styles is reflected at the test page http://get.adobe.com/flashplayer/about/).

both of the styles are attached via <link>, i.e.

<link rel="stylesheet" type="text/css" href="chrome://pluginproblem/content/pluginProblemContent.css" />
<link rel="stylesheet" type="text/css" href="chrome://mozapps/skin/plugins/pluginProblem.css" />

both urls are loaded via addressbar. Changing links order doesn't seem have any effect. Any advice, what could be wrong here?

        Attachment #9020280 -
        Attachment is obsolete: true
Flags: needinfo?(bgrinstead)

    
    

    
  
Comment on attachment 9021746 [details] [diff] [review]
part1: move AttachAndSetUAShadowRoot to generic HTML element

Shadow DOM isn't attached to a tag name. It is attached to an element.
So perhaps call it CanAttachShadowDOM()

   // Shadow DOM v1
+  bool CanAttachShadowToTagName() const;
   already_AddRefed<ShadowRoot> AttachShadow(const ShadowRootInit& aInit,
                                             ErrorResult& aError);

Since the new method isn't part of any spec, move it below AttachShadow so that v1 comment is still correct.

MOZ_DIAGNOSTIC_ASSERT is probably too slow. MOZ_ASSERT is enough in AttachAndSetUAShadowRoot.

+  // Add a closed shadow root to host video controls comment is wrong. Just drop the comment.

        Attachment #9021746 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to alexander :surkov (:asurkov) from comment #38)
> both of the styles are attached via <link>, i.e.
> 
> <link rel="stylesheet" type="text/css"
> href="chrome://pluginproblem/content/pluginProblemContent.css" />
> <link rel="stylesheet" type="text/css"
> href="chrome://mozapps/skin/plugins/pluginProblem.css" />
> 
> both urls are loaded via addressbar. Changing links order doesn't seem have
> any effect. Any advice, what could be wrong here?

Perhaps you would need to mark it as contentaccessible=yes?

https://searchfox.org/mozilla-central/search?q=contentaccessible%3Dyes&case=false&regexp=false&path= 

but it should be marked in the first place for in-content XBL binding to access it... so I have no idea why. Are you sure XBL pluginProblem actually loads the stylesheet without problem?

    
    

    
  
In addition to what Tim mentioned in Comment 40, a couple thoughts:

- If you grab the contents of the file that isn't working and paste it into the one that is, does that work as you expected? Wouldn't be a permanent fix but could help debug
- It's interesting that this explicitly sets the HTML namespace: https://searchfox.org/mozilla-central/source/toolkit/themes/shared/plugins/pluginProblem.css#5. That seems unnecessary, can you check and see if removing that (and any references to `html|`) changes anything?
Flags: needinfo?(bgrinstead)

    
    

    
  
(In reply to ExE Boss from comment #10)
> This needs to be negated, like so:
> 
> ```css
> @supports not -moz-bool-pref("dom.ua_widget.enabled") {
>     …
> }
> ```

From testing out bug 1425874 locally, I just realized this needs to be 

@supports not -moz-bool-pref("dom.ua_widget.enabled") or not -moz-bool-pref("dom.webcomponents.shadowdom.enabled") {

because UA Widget depends on Shadow DOM.

I've also realized this has no effect until reboot, which means XBL won't be applied until reboot if the user opt-out of UA Widget with the pref.

    
    

    
  
(Sorry, the correct syntax according to MDN should be

@supports (not -moz-bool-pref("dom.ua_widget.enabled")) or (not -moz-bool-pref("dom.webcomponents.shadowdom.enabled")) {

)

    
    

    
  
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f76aad81aa
add nsGenericHTMLElement::AttachAndSetUAShadowRoot helper method, r=smaug

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #40)
> (In reply to alexander :surkov (:asurkov) from comment #38)
> > both of the styles are attached via <link>, i.e.
> > 
> > <link rel="stylesheet" type="text/css"
> > href="chrome://pluginproblem/content/pluginProblemContent.css" />
> > <link rel="stylesheet" type="text/css"
> > href="chrome://mozapps/skin/plugins/pluginProblem.css" />
> > 
> > both urls are loaded via addressbar. Changing links order doesn't seem have
> > any effect. Any advice, what could be wrong here?
> 
> Perhaps you would need to mark it as contentaccessible=yes?

I tried but it seems doesnt' have any effect

> https://searchfox.org/mozilla-central/
> search?q=contentaccessible%3Dyes&case=false&regexp=false&path= 
> 
> but it should be marked in the first place for in-content XBL binding to
> access it... so I have no idea why. Are you sure XBL pluginProblem actually
> loads the stylesheet without problem?

I copied css files to global/plugins and it works. Curious, if plugin styles should be hosted by mozapps, and whether they can be moved to either chrome://pluginproblem or chrome://global. Does it sound reasonable?

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #43)
> (Sorry, the correct syntax according to MDN should be
> 
> @supports (not -moz-bool-pref("dom.ua_widget.enabled")) or (not
> -moz-bool-pref("dom.webcomponents.shadowdom.enabled")) {
> 
> )

thanks, changed locally

    
    

    
  
(In reply to alexander :surkov (:asurkov) from comment #45)
> > but it should be marked in the first place for in-content XBL binding to
> > access it... so I have no idea why. Are you sure XBL pluginProblem actually
> > loads the stylesheet without problem?
> 
> I copied css files to global/plugins and it works. Curious, if plugin styles
> should be hosted by mozapps, and whether they can be moved to either
> chrome://pluginproblem or chrome://global. Does it sound reasonable?

If I move those css and imgs under chrome://global/plugins from chrome://mozapps/plugins, then would it cause any problems or would anyone had any concerns?
Flags: needinfo?(timdream)

    
    

    
  
We would still like to keep the XBL binding working and the easiest way to do that is not to touch it. I would avoid doing that unless there aren't any other ways.
Flags: needinfo?(timdream)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        wip2 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #49)
> We would still like to keep the XBL binding working and the easiest way to
> do that is not to touch it.

there's a number of them (https://searchfox.org/mozilla-central/search?q=chrome%3A%2F%2Fmozapps%2F.%2B%2Fplugins&case=false&regexp=true&path=), it's probably not hard to change them all, but otherwise I agree it's better to not change if possible.

> I would avoid doing that unless there aren't any
> other ways.

I feel lost, I need help from someone who knows how styling works. Here's my findings so far:

* pluginProblem.css is loaded if moved under 'global' from 'mozapps'
* contentaccessible=yes added to %skin/classic/mozapps/ in jar.mn doesn't have any effect
* adding .manifest file having "category agent-style-sheets" for pluginProblem.css makes it visible for UIWidget, but it doens't load any of images referred by css file, even chrome://global/skin/icons/close.svg (inspired by https://searchfox.org/mozilla-central/source/toolkit/pluginproblem/pluginGlue.manifest)

Tim, looking for any pointers or advice where to go whom to ask.

        Attachment #9021779 -
        Attachment is obsolete: true
Flags: needinfo?(timdream)

    
    

    
  
I would fire up the debugger and step into the stylesheet loader to see why it refuse to load the sheet. It would take some time but you will eventually find the cause. If that's too much investment of time I do think it makes sense to move the file if your reviewer agrees.

Noted that if we are shipping UA Widget on 65 outright (provided that SD and UA Widget pref are all removed; see bug 1503019) it would not make sense to keep XBL continue working.
Flags: needinfo?(timdream)

    
    

    
  
I'd also like to know why this isn't working, but moving from chrome://mozapps/skin/plugins to chrome://global/skin/plugins or chrome://pluginproblem/skin seems pretty reasonable to me. We aren't even consistently using "mozapps" for the "content" portion - we're using chrome://pluginproblem/content/ in that case. And I don't see why doing that would cause any breakage for the XBL implementation.

I'd do that in a separate bug though, to lower the number of moving parts here.

    
    

    
  
Here's a try push that throws in the plugin problem constructor, to see if it's used on android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a92b35b57cb9060f8ae69e53fbad8d0d1ce02ae6

    
    

    
  
I am asked to take this of Alex's plate. Let me see what's needed to finish up his WIP.
Assignee: surkov.alexander → timdream
Status: NEW → ASSIGNED

    
    

    
  
Well, bug 1503019 will vastly simplify the `@supports not ‑moz‑bool‑pref(…)` condition.
Depends on: 1503019

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #53)
> Here's a try push that throws in the plugin problem constructor, to see if
> it's used on android:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a92b35b57cb9060f8ae69e53fbad8d0d1ce02ae6

According to this, pluginProblems UI is not used in Android and its specific styles can be removed.

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #56)
> (In reply to Brian Grinstead [:bgrins] from comment #53)
> > Here's a try push that throws in the plugin problem constructor, to see if
> > it's used on android:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=a92b35b57cb9060f8ae69e53fbad8d0d1ce02ae6
> 
> According to this, pluginProblems UI is not used in Android and its specific
> styles can be removed.

That would certainly simplify things. Nick, can you either confirm or point us to someone who could that we don't use plugins in Fennec? I believe for desktop the only remaining consumer of this "plugin problem" UI is Flash, although AIUI the UI used to be used for other Plugins when desktop supported them.
Flags: needinfo?(nalexander)

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #57)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #56)
> > (In reply to Brian Grinstead [:bgrins] from comment #53)
> > > Here's a try push that throws in the plugin problem constructor, to see if
> > > it's used on android:
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=a92b35b57cb9060f8ae69e53fbad8d0d1ce02ae6
> > 
> > According to this, pluginProblems UI is not used in Android and its specific
> > styles can be removed.
> 
> That would certainly simplify things. Nick, can you either confirm or point
> us to someone who could that we don't use plugins in Fennec? I believe for
> desktop the only remaining consumer of this "plugin problem" UI is Flash,
> although AIUI the UI used to be used for other Plugins when desktop
> supported them.

It looks like the mobile pluginProblem.css file was last significantly changed in https://bugzilla.mozilla.org/show_bug.cgi?id=1307445#c7, so also setting needinfo for Sebasian.
Flags: needinfo?(s.kaspari)

    
    

    
  
Support of plugin on Android was removed way back in bug 1381916. I guess the remaining mobile-specific files are all leftovers (e.g. there is no reference anywhere to PluginHelper other than itself). I will remove them in a separate patch.

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #59)
> Support of plugin on Android was removed way back in bug 1381916. I guess
> the remaining mobile-specific files are all leftovers (e.g. there is no
> reference anywhere to PluginHelper other than itself). I will remove them in
> a separate patch.

Alright, that sounds definitive enough, combined with the lack of automated tests using the binding. I'll clear the needinfos and any concerns can be raised in a review on that separate patch.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(nalexander)

    
    

    
  


  
These styles and PluginHelper.js should be removed back in bug 1381916.

    
    

    
  


  
This is needed because UA Widget cannot load resources from chrome://mozapps.

Depends on D11701

    
    

    
  


  
The :-moz-handler selectors are broken in the sheets, need to find out why.

Depends on D11702

        Attachment #9022824 -
        Attachment is obsolete: true

    
    

    
  
Ah, these patches should be Part III-V...

Anyway, I am still figuring out the problem as said in comment 63.

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #59)
> Support of plugin on Android was removed way back in bug 1381916. I guess
> the remaining mobile-specific files are all leftovers (e.g. there is no
> reference anywhere to PluginHelper other than itself). I will remove them in
> a separate patch.

This is correct; anything left is vestigial.

        Attachment #9024592 -
        Attachment description: Bug 1497940 - Part I, Remove unused pluginProblem mobile styles → Bug 1497940 - Part III, Remove unused pluginProblem mobile styles

        Attachment #9024593 -
        Attachment description: Bug 1497940 - Part II, Move pluginProblem resources from chrome://mozapps to chrome://global → Bug 1497940 - Part IV, Move pluginProblem resources from chrome://mozapps to chrome://global

        Attachment #9024594 -
        Attachment description: Bug 1497940 - Part III, Convert pluginProblem to UA Widget → Bug 1497940 - Part V, Convert pluginProblem to UA Widget

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #69)
> These should work.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=758f50906e0547319ab2c9ff32379befea190402

Asides from eslint failure (fixed) I don't see any problems.

This is ready to review though :smaug said he will not be able to get to it this week. I've flagged him for review for now, if we still have an owner to Plugin UI I would like to redirect this to him so we can stop overloading :smaug.

    
  
Blocks: 1507894

    
    

    
  
There is still an issue with Windows-only Hang UI.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6efb5bfbd6f91c0f2c9d112ded63dafc6211cf97&selectedJob=212680801

It complained about not finding a minidump file.

I've looked at the impl (bug 805591) it doesn't seem to depend on anything on the front-end directly. Strangely the test observer does get a non-empty dump ID. It is set from here. Could CrashReporterHost be lying?

https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/dom/plugins/ipc/PluginModuleParent.cpp#1547

    
    

    
  
If I removed the binding from the Windows artifact build (w/o actually testing out this bug), I can see the test still passes.

    
    

    
  
My current suspicion b/t difference of (a) UA Widget, (b) the XBL binding, and (c) nothing, is this

https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/pluginproblem/content/pluginProblem.xml#67

We are replacing the content of the plug-in w/o a <slot /> in (a), while there is a <children /> wrapping inside a display: none <div>. I wonder why we needed that in the XBL binding...

    
    

    
  
... and I cannot reproduce it on my local opt build on Windows 10.

Let's see what requestCompleteLog() shows, on the same m-c base.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cc3cd086fe198dbd480e5db2c6dc4bd1d505382

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #76)
> For some reason, requestCompleteLog() didn't work.
> 
> Try run with MOZ_LOG=IPCPlugins:5
Doesn't seem to be useful -- the debugging messages and the test log didn't line up

> Try run with that and dom.ua_widget.enabled=false
The test passed.

> Try run without any crash UI (skipping UA Widget construction)
Failed

At least we know the XBL binding didn't break, and the crash reporter relies on the binding in some way...

Disable UA Widget, replace pluginProblem with an empty binding:
https://treeherder.mozilla.org/#/jobs?repo=try&searchStr=mochitest-chrome-2&selectedJob=212993817&revision=9e6c16930f9ef8a24e266aa73120e1eb98072b7f

With UA Widget *and* XBL binding both enabled, replace pluginProblem with an empty binding:
https://treeherder.mozilla.org/#/jobs?repo=try&searchStr=mochitest-chrome-2&selectedJob=212993817&revision=47fc2b81708c445d074293308e36e7d0753f32f5

    
    

    
  
waitForCondition() in the testObserver *and* also asks test functions to wait for the observer function to complete.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ba4787210805c256dfb2b47a10d60c706ec6d5f

    
    

    
  
Hi Aaron, would you be able to give us some insight on the test file dom/plugins/test/mochitest/test_hangui.xul ?

So when the "plugin-crashed" observer notification happens, the testObserver here asserts the existence of the minidump files.

https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/dom/plugins/test/mochitest/hang_test.js#21

For some reason changing the front-end pluginProblem UI changes the timing: according to the test log it receives a dump ID but couldn't find the file. I can see the file handle of the minidump file is created here

https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/crashreporter/nsExceptionHandler.cpp#3902

and passed down, and the dump ID is derived from the file path. I fail to understand where in the code base that the file is actually written.

Is there a possibility that a race exists somewhere that cause us to receive a dump ID before the file is actually written? If so, is that something worth fixing, or we should just workaround it if the push in comment 79 works? Thanks for the input.
Flags: needinfo?(aklotz)

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #79)
> waitForCondition() in the testObserver *and* also asks test functions to
> wait for the observer function to complete.

This didn't work. So it's not just a timing issue and the files are really missing. :'(

    
    

    
  
I wonder if investigations made in bug 1006234 is useful or not.

    
    

    
  
I am pretty sure the timing of PluginBindingAttached event should be unrelated as PluginChild.jsm, responding to the event, only decorate the UI, but here is the try pushes that touches the event

Disable UA Widget and run an XBL binding containing only the constructor dispatching the event
https://treeherder.mozilla.org/#/jobs?repo=try&searchStr=windows%2Cmochitest-chrome-2&selectedJob=213035619&revision=be2572b7e6a37fc12a33299a08f7e106cafb8027

Put the event dispatching inside a setTimeout(), "restoring" the event order.
https://treeherder.mozilla.org/#/jobs?repo=try&searchStr=windows%2Cmochitest-chrome-2&selectedJob=213035619&revision=2300b4cc6225718238356233c8759838360a8bd6

    
    

    
  
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #82)
> I wonder if investigations made in bug 1006234 is useful or not.

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1006234#c235, I wonder if the test will only fail when running with the rest of the chunk it's in. So far I've only attempted to reproduce running the test on its own.

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #84)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #82)
> > I wonder if investigations made in bug 1006234 is useful or not.
> 
> Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1006234#c235, I wonder
> if the test will only fail when running with the rest of the chunk it's in.
> So far I've only attempted to reproduce running the test on its own.

I think the previous test is trying to submit the report by digging into the XBL anonymous content of the plugin problem UI, which throws and then somehow prevents the test from properly cleaning up - leading to errors like:

01:37:48     INFO - GECKO(3476) | JavaScript error: resource://specialpowers/SpecialPowersObserverAPI.js, line 201: NS_ERROR_FILE_IS_LOCKED: Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED) [nsIFile.remove 

at https://searchfox.org/mozilla-central/source/testing/specialpowers/content/SpecialPowersObserverAPI.js#201.

I think if this line is updated to get the correct button it should fix it: https://searchfox.org/mozilla-central/rev/8f89901f2d69d9783f946a7458a6d7ee70635a94/dom/plugins/test/mochitest/test_hang_submit.xul#122

    
    

    
  
It looks like there's two instances of the anonymous node being selected in this test folder: https://searchfox.org/mozilla-central/search?q=getanonymous&path=dom%2Fplugins%2Ftest
Flags: needinfo?(timdream)

    
    

    
  
😱😱😱😱😱

Nice find. I can confirm the tests passed locally with that fix.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c3401a05e3c81282af06be6b7fa52e779c3923c

Let's wait for this try result before checking in.
Flags: needinfo?(timdream)
Flags: needinfo?(aklotz)

    
    

    
  
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6918f8e2f38c
Part III, Remove unused pluginProblem mobile styles r=snorp
https://hg.mozilla.org/integration/autoland/rev/b503b1a1552c
Part IV, Move pluginProblem resources from chrome://mozapps to chrome://global r=mossop
https://hg.mozilla.org/integration/autoland/rev/493083d55865
Part V, Convert pluginProblem to UA Widget r=smaug

    
    

    
  
Backed out 3 changesets (bug 1497940) for Browser-chrome in toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=213310359&repo=autoland&lineNumber=7001

INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | part3: state menu should have 'Ask To Activate' selected - 
[task 2018-11-22T02:49:14.610Z] 02:49:14     INFO - Console message: [JavaScript Error: "this.window is undefined; can't access its "DOMParser" property" {file: "chrome://global/content/elements/pluginProblem.js" line: 16}]
[task 2018-11-22T02:49:14.610Z] 02:49:14     INFO - @chrome://global/content/elements/pluginProblem.js:16:11
[task 2018-11-22T02:49:14.611Z] 02:49:14     INFO - setupWidget@resource://gre/actors/UAWidgetsChild.jsm:84:16
[task 2018-11-22T02:49:14.611Z] 02:49:14     INFO - setupOrNotifyWidget@resource://gre/actors/UAWidgetsChild.jsm:38:7
[task 2018-11-22T02:49:14.612Z] 02:49:14     INFO - handleEvent@resource://gre/actors/UAWidgetsChild.jsm:23:9
[task 2018-11-22T02:49:14.612Z] 02:49:14     INFO - handleActorEvent@resource://gre/modules/ActorManagerChild.jsm:134:26
[task 2018-11-22T02:49:14.612Z] 02:49:14     INFO - EventListener.handleEvent*addEventListener@resource://gre/modules/ActorManagerChild.jsm:83:5
[task 2018-11-22T02:49:14.613Z] 02:49:14     INFO - init@resource://gre/modules/ActorManagerChild.jsm:56:7
[task 2018-11-22T02:49:14.613Z] 02:49:14     INFO - attach@resource://gre/modules/ActorManagerChild.jsm:306:48
[task 2018-11-22T02:49:14.614Z] 02:49:14     INFO - @chrome://global/content/browser-content.js:14:1
[task 2018-11-22T02:49:14.614Z] 02:49:14     INFO - 
[task 2018-11-22T02:49:14.615Z] 02:49:14     INFO - Buffered messages finished
[task 2018-11-22T02:49:14.616Z] 02:49:14     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | Uncaught exception - part4: should have a click-to-play notification - timed out after 50 tries.
[task 2018-11-22T02:49:14.617Z] 02:49:14     INFO - Leaving test bound 
[task 2018-11-22T02:49:14.619Z] 02:49:14     INFO - GECKO(4248) | --DOMWINDOW == 3 (0xe83bac00) [pid = 4328] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-11-22T02:49:14.620Z] 02:49:14     INFO - GECKO(4248) | --DOMWINDOW == 2 (0xf7169bc0) [pid = 4328] [serial = 1] [outer = (nil)] [url = about:blank]
[task 2018-11-22T02:49:14.621Z] 02:49:14     INFO - GECKO(4248) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2018-11-22T02:49:14.622Z] 02:49:14     INFO - GECKO(4248) | MEMORY STAT | vsize 637MB | residentFast 275MB | heapAllocated 80MB
[task 2018-11-22T02:49:14.623Z] 02:49:14     INFO - TEST-OK | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | took 12412ms
[task 2018-11-22T02:49:14.624Z] 02:49:14     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-11-22T02:49:14.625Z] 02:49:14     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | Found an unexpected tab at the end of test run: http://127.0.0.1:8888/browser/toolkit/mozapps/extensions/test/browser//plugin_test.html - 
[task 2018-11-22T02:49:14.651Z] 02:49:14     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-11-22T02:49:14.654Z] 02:49:14     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | Found an unexpected tab at the end of test run: about:addons - 
[task 2018-11-22T02:49:14.754Z] 02:49:14     INFO - GECKO(4248) | [Parent 4248, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005: file /builds/worker/workspace/build/src/docshell/shistory/nsSHistory.cpp, line 1292
[task 2018-11-22T02:49:14.754Z] 02:49:14     INFO - GECKO(4248) | [Parent 4248, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005: file /builds/worker/workspace/build/src/docshell/shistory/nsSHistory.cpp, line 1292
[task 2018-11-22T02:49:14.771Z] 02:49:14     INFO - GECKO(4248) | ++DOCSHELL 0xe7f51000 == 1 [pid = 4488] [id = {b7945bf5-c28b-4b0e-aeef-5dbc6b06267d}]
[task 2018-11-22T02:49:14.854Z] 02:49:14     INFO - checking window state
[task 2018-11-22T02:49:14.895Z] 02:49:14     INFO - GECKO(4248) | ++DOMWINDOW == 1 (0xf7169a80) [pid = 4488] [serial = 1] [outer = (nil)]
[task 2018-11-22T02:49:14.956Z] 02:49:14     INFO - TEST-START | toolkit/mozapps/extensions/test/browser/browser_bug523784.js
[task 2018-11-22T02:49:14.997Z] 02:49:14     INFO - GECKO(4248) | ++DOCSHELL 0xddb7e800 == 10 [pid = 4248] [id = {9ff847c5-853f-4fb2-a0d1-5962ec0b4d69}]
[task 2018-11-22T02:49:14.998Z] 02:49:14     INFO - GECKO(4248) | ++DOMWINDOW == 19 (0xde56c300) [pid = 4248] [serial = 28] [outer = (nil)]
[task 2018-11-22T02:49:14.999Z] 02:49:14     INFO - GECKO(4248) | ++DOMWINDOW == 20 (0xddf80c00) [pid = 4248] [serial = 29] [outer = 0xde56c300]
[task 2018-11-22T02:49:15.035Z] 02:49:15     INFO - GECKO(4248) | JavaScript error: resource:///modules/AsyncTabSwitcher.jsm, line 743: Error: TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "FX_TAB_SWITCH_COMPOSITE_E10S_MS", key: ""
[task 2018-11-22T02:49:15.072Z] 02:49:15     INFO - GECKO(4248) | ++DOMWINDOW == 2 (0xe8496800) [pid = 4488] [serial = 2] [outer = 0xf7169a80]
[task 2018-11-22T02:49:15.149Z] 02:49:15     INFO - GECKO(4248) | [Parent 4248, Main Thread] WARNING: NS_ENSURE_TRUE(root) failed: file /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp, line 3506
[task 2018-11-22T02:49:15.168Z] 02:49:15     INFO - GECKO(4248) | [Parent 4248, Main Thread] WARNING: Error loading theme icon 'dialog-error' for stock: Icon 'dialog-error' not present in theme ubuntu-mono-dark: 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141
[task 2018-11-22T02:49:15.169Z] 02:49:15     INFO - GECKO(4248) | (firefox:4248): Gtk-WARNING **: Error loading theme icon 'dialog-error' for stock: Icon 'dialog-error' not present in theme ubuntu-mono-dark
[task 2018-11-22T02:49:15.334Z] 02:49:15     INFO - GECKO(4248) | ++DOCSHELL 0xddf81800 == 11 [pid = 4248] [id = {0acafa36-91e1-4a51-beea-0042cf629f68}]
[task 2018-11-22T02:49:15.334Z] 02:49:15     INFO - GECKO(4248) | ++DOMWINDOW == 21 (0xde56c940) [pid = 4248] [serial = 30] [outer = (nil)]
[task 2018-11-22T02:49:15.334Z] 02:49:15     INFO - GECKO(4248) | ++DOMWINDOW == 22 (0xddf85400) [pid = 4248] [serial = 31] [outer = 0xde56c940]
[task 2018-11-22T02:49:15.350Z] 02:49:15     INFO - GECKO(4248) | [Parent 4248, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805D0021: file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 1142
[task 2018-11-22T02:49:15.371Z] 02:49:15     INFO - GECKO(4248) | [Parent 4248, Main Thread] WARNING: NS_ENSURE_TRUE(root) failed: file /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp, line 3506
[task 2018-11-22T02:49:15.529Z] 02:49:15     INFO - GECKO(4248) | MEMORY STAT | vsize 637MB | residentFast 275MB | heapAllocated 80MB
[task 2018-11-22T02:49:15.530Z] 02:49:15     INFO - TEST-OK | toolkit/mozapps/extensions/test/browser/browser_bug523784.js | took 561ms
[task 2018-11-22T02:49:15.586Z] 02:49:15     INFO - GECKO(4248) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpXN5v6S.mozrunner/runtests_leaks_tab_pid4508.log
[task 2018-11-22T02:49:15.791Z] 02:49:15     INFO - GECKO(4248) | --DOMWINDOW == 6 (0xe7fadc00) [pid = 4309] [serial = 12] [outer = (nil)] [url = moz-extension://e9ed1fd9-902a-4d10-8dd6-760c0130cc2a/_generated_background_page.html]
[task 2018-11-22T02:49:15.909Z] 02:49:15     INFO - GECKO(4248) | --DOMWINDOW == 2 (0xe84a2c00) [pid = 4418] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-11-22T02:49:15.910Z] 02:49:15     INFO - GECKO(4248) | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2018-11-22T02:49:15.938Z] 02:49:15     INFO - checking window state
[task 2018-11-22T02:49:16.184Z] 02:49:16     INFO - TEST-START | toolkit/mozapps/extensions/test/browser/browser_bug562797.js

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=493083d55865df594eef9f26c5ba7a98873cacb1

Backout:
https://hg.mozilla.org/integration/autoland/rev/373839b9f787786aca3869614d71bcda66bfabf4
Flags: needinfo?(timdream)

    
    

    
  
I was tricked by eslint to use chrome-only ownerGlobal property. Let me verify that locally before triggering autoland again.
Flags: needinfo?(timdream)

    
    

    
  
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20d7a96fca5d
Part III, Remove unused pluginProblem mobile styles r=snorp
https://hg.mozilla.org/integration/autoland/rev/2eeaf4aca069
Part IV, Move pluginProblem resources from chrome://mozapps to chrome://global r=mossop
https://hg.mozilla.org/integration/autoland/rev/5f543ba66e2c
Part V, Convert pluginProblem to UA Widget r=smaug

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/20d7a96fca5d
https://hg.mozilla.org/mozilla-central/rev/2eeaf4aca069
https://hg.mozilla.org/mozilla-central/rev/5f543ba66e2c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox65:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1509297
See Also:  → 1509291

    
  
Type: enhancement → task

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: