Element picker breaks after I try to pick an element in Youtube HTML5 embed object

Categories

(DevTools :: Inspector, defect, P1)

For bugs in Firefox DevTools, the developer tools within the Firefox web browser. This includes issues about the user interface of the toolbox, special pages such as about:debugging and about:devtools, and developer-related APIs.

I investigated this a little bit and I'm going to send this in Matteo's way because he knows that part of the code better.
In getAdjustedQuads (devtools\shared\layout\utils.js), we receive a 'boundaryWindow' argument. In the case I tested here (1C), this 'boundaryWindow' is in fact an <embed> tag, not an <iframe>.
So later in the code, in getIframeContentOffset, 'aIframe.contentWindow' fails because aIframe isn't an <iframe>.
Matteo, could you please take a look at this?

Yeah, there's a good chance the youtube rewrite causes this. A much simpler STR:
1. Create an HTML file with an embed block in it, e.g.
<object>
<embed src="https://www.youtube.com/v/pZ5w3UpwtwA"
type="application/x-shockwave-flash"
allowscriptaccess="always"
width="422" height="258"></embed>
</object>
2. Open that file in either nightly or aurora, open dev tools, try to choose element via element picker.
So basically what happens in the rewrite is that we render either object OR embed tags that try to load youtube flash as we would an object tag with text/html, which means we treat it like an iframe, basically. If the tag is just an object, e.g.
<object width="640" height="390" data="https://www.youtube.com/v/pZ5w3UpwtwA"> </object>
things seem to work fine even after rewrite. It's just when it's an embed tag that things seem to break.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> A much simpler STR:
I don't want a long debate (mark as spam), but I provided such a long steps for 2 reasons:
1) There were several times when people couldn't reproduce my bugs with normal steps
2) "Severity"
Actually, STR in comment 6 could easily be parried by Firefox developers, because "normally
nobody should debug Youtube content except for Youtube devs". My steps show that
even hovering Youtube content can temporarily break element picker on your own site

So the highlighter today doesn't work at all on <embed>, this is something we need to fix quickly.
When the highlighter wants to highlight something in a given window, it tries to get the container frame element of that window if it isn't a top-level one.
It does this with nsIDOMWindowUtils.getContainerElement.
If a window happens to be in an <embed>, then the containerElement will be this <embed> element.
However, the highlighter today just assumes that all containerElements are things we can get 'contentWindow' on, like an <iframe>.
This isn't the case with <embed>s.
The only other solution I found to access the content window in all cases is by using the inIDeepTreeWalker. We already use this API in the inspector to display the DOM tree in the markup-view, so it works well. Maybe there are other solutions, but I'm not aware of them.
I will attach a patch with this fix in.

Comment on attachment 8726801[details]
MozReview Request: Bug 1246088 - Safely access contentWindow in iframe and embed; r=bgrins
https://reviewboard.mozilla.org/r/38239/#review35473
This fixes the problem.. weird that there isn't a way to dig into the window on an embed without resorting to the deeptreewalker. Can you please re-request review after any changes so I can take one more look at it?
::: devtools/client/inspector/test/browser_inspector_highlighter-embed.js:24
(Diff revision 1)
> + ok(true);
Could we check to see if body is the inspector.selection's selected node?
::: devtools/shared/layout/utils.js:373
(Diff revision 1)
> +function safelyGetContentWindow(aIframe) {
This should be called aFrame instead now
::: devtools/shared/layout/utils.js:386
(Diff revision 1)
> + return document.defaultView;
We should be a little defensive here I think - and check at least that 'document' exists before accessing a prop on it (we could also check the nodeType to make sure it's a Document but that might be unnecessary since i'd expect defaultView to not be defined if it wasn't a document)
::: devtools/shared/layout/utils.js:388
(Diff revision 1)
> +export.safelyGetContentWindow = safelyGetContentWindow;
export. -> exports.
::: devtools/shared/layout/utils.js:404
(Diff revision 1)
> function getIframeContentOffset(aIframe) {
Instances of iframe should probably be renamed to frame now - getFrameContentOffset, aFrame

(In reply to Brian Grinstead [:bgrins] from comment #11)
> ::: devtools/client/inspector/test/browser_inspector_highlighter-embed.js:24
> (Diff revision 1)
> > + ok(true);
>
> Could we check to see if body is the inspector.selection's selected node?
Will do, sorry I didn't think of doing this.
> ::: devtools/shared/layout/utils.js:373
> (Diff revision 1)
> > +function safelyGetContentWindow(aIframe) {
>
> This should be called aFrame instead now
Makes sense. I was just trying to be consistent with the rest of the file, but I'll change this.
> ::: devtools/shared/layout/utils.js:386
> (Diff revision 1)
> > + return document.defaultView;
>
> We should be a little defensive here I think - and check at least that
> 'document' exists before accessing a prop on it (we could also check the
> nodeType to make sure it's a Document but that might be unnecessary since
> i'd expect defaultView to not be defined if it wasn't a document)
What would you expect to happen if document is undefined or not a document?
Returning null for example would force its consumer (getIframeContentOffset) to do something about it, but what?
I think this should remain an error state, but maybe we can add a developer-friendly error message, like:
if (!document || !document.defaultView) {
throw new Error("Couldn't get the content window inside frame " + frame);
}
> ::: devtools/shared/layout/utils.js:388
> (Diff revision 1)
> > +export.safelyGetContentWindow = safelyGetContentWindow;
>
> export. -> exports.
Whoops, I wonder how this even worked when I tested it locally?! Anyhow, this function isn't required outside of this module for now, I'll remove the export.
> ::: devtools/shared/layout/utils.js:404
> (Diff revision 1)
> > function getIframeContentOffset(aIframe) {
>
> Instances of iframe should probably be renamed to frame now -
> getFrameContentOffset, aFrame
Will do.

Comment on attachment 8726801[details]
MozReview Request: Bug 1246088 - Safely access contentWindow in iframe and embed; r=bgrins
Approval Request Comment
[Feature/regressing bug #]: Comment 3 and comment 6 mention the flash-to-html5 rewrite as being the cause for this. I do not know the bug number for that.
[User impact if declined]: If declined, the inpsector cannot be used to inspect elements inside embedded HTML5 youtube videos.
[Describe test coverage new/current, TreeHerder]: The change has been on m-c for 1,5 week with a new automated test. So this is stable so far.
[Risks and why]: This is a pretty central piece of code in the devtools inspector. It is used anytime a node is highlighted in the page, so basically anytime you use the inspector. So, if the change is buggy, then that will have a major impact. But, having said this, the change seem pretty straighforward, and I believe we would have caught regressions on m-c by now. So I believe the risk is low.
[String/UUID change made/needed]: None

The risk assessment provided here is amazing as it's very helpful and well thought out. Thank you for that! I'll wait for a day or two to get verification of the fix on Nightly before uplifting to Aurora and Beta.

Fixed on: Win7_64, Nightly 48, 32bit, ID 20160323030400
I followed STR in comment 0 and also checked some other testcases - and everything seems to work fine
Now, element picker not only doesn't break at Step 4, but also allows to inspect Youtube HTML5 player

(In reply to arni2033 from comment #23)
> Fixed on: Win7_64, Nightly 48, 32bit, ID 20160323030400
>
> I followed STR in comment 0 and also checked some other testcases - and
> everything seems to work fine
> Now, element picker not only doesn't break at Step 4, but also allows to
> inspect Youtube HTML5 player
Awesome! Thank you as always for your prompt verification. :)

I can't really assess the importance of fixing this in beta. It's hard to guess how many people will try to inspect elements inside an embedded youtube object in firefox during the timeframe of 46.
Based on this, I would rather play it safe and wontfix it for 46?
Crafting a custom patch shouldn't be hard, but still has risks I guess.