Closed Bug 1246088 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox44 unaffected, firefox45 unaffected, firefox46+ unaffected, firefox47+ fixed, firefox48 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 + unaffected
firefox47 + fixed
firefox48 --- verified

People

(Reporter: arni2033, Assigned: pbro)

References

Details

(Keywords: regression, Whiteboard: [btpp-fix-now])

Attachments

(2 files)

MozReview Request: Bug 1246088 - Safely access contentWindow in iframe and embed; r=bgrins
58 bytes, text/x-review-board-request
bgrins
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1246088 - ESLint cleanups of devtools/shared/layout/utils.js; r=bgrins
58 bytes, text/x-review-board-request
bgrins
: review+
Details 
>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160204030229
STR:
1.A) Open http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276 (from bug 1240471)
     Scroll the page so that the Youtube HTML5 embed object in the first post in that thread
     was aligned at the center of content area (vertically)
1.B) Open the following "data:" url
>   data:text/html,<div><div><div style="height:340px; width:50px; background:red;"></div><embed src="http://www.youtube.com/v/eoUf1bbMTa8" width="560" height="340"><style>div{display:inline-block;}
1.C) Open the following "data:" url
>   data:text/html,<div><div><div style="height:340px; width:50px; background:red;"></div><embed src="http://www.youtube.com/embed/eoUf1bbMTa8" width="560" height="340"><style>div{display:inline-block;}

2. Open devtools, dock the toolbox to the bottom side of window, resize it so that the Youtube
   HTML5 embed object was partially clipped by toolbox, but red play button was still visible
3. Switch to Inspector tab, click Element picker.
4. Move mouse pointer to the red play button in the Youtube HTML5 embed object. In this step, make
   sure that mouse doesn't move over anything else on the page but the Youtube HTML5 embed object
5. Click left mouse button
6. Click Element picker again
7.A) Move mouse to the red play button in the same manner as in Step 5.
     Then move it in a straight line to the 1st STALKER icon ("radiation") in the poster's info
7.B) Move mouse to the red area on the left
7.C) Move mouse to the red area on the left (the same as 7.B)
8. Click left mouse button

AR: After Step 4 and Step 8 nothing happens.

ER: Either X or Y
 X) After Step 4 the youtube button in markup should be selected in markup
 Y) Ok, let it fail in Step 4, but Element picker at least shouldn't be broken
    and the element from Step 7 should be selected in markup after Step 8.

I don't know if this can be called "regression". Change ranges:
> (A) & (B)   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=28a2f47fced812306c72e4039ecb477bbb5c47fa&tochange=24e0645f9c31e717586e287adb863e5c3f68c2f3
> (C)         https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d353d43108373a8461ce304a15c23084c8eafb6d&tochange=bc2fc3ae3bd25f619102e8e2ce1cae909280fe2d

The mechanism is very similar to the one described by me in bug 1228675
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?
Flags: needinfo?(zer0)
That's odd, I'll take a look!
Assignee: nobody → zer0
Flags: needinfo?(zer0)
Kyle, this bug looks like a regression from rewriting YouTube embeds.
Flags: needinfo?(kyle)
Matteo, do you think this element picker problem should block the release of our Flash-to-HTML5 YouTube embed rewriter?
status-firefox44: --- → unaffected
status-firefox45: --- → unaffected
status-firefox46: --- → affected
Flags: needinfo?(zer0)
Keywords: regression
Tracking for 46+ since this is a recent regression.
tracking-firefox46: --- → +
tracking-firefox47: --- → +
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.
Flags: needinfo?(kyle)
(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.
Flags: needinfo?(zer0)
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Assignee: zer0 → pbrosset
Status: NEW → ASSIGNED
I wanted to give this to bgrins for review but he's off until next Monday and not accepting any reviews until then. I will ping him when he gets back.
Attachment #8726801 - Flags: review?(bgrinstead)
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
Attachment #8726801 - Flags: review?(bgrinstead)
(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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38239/diff/1-2/
Attachment #8726801 - Flags: review?(bgrinstead)
Attachment #8726801 - Flags: review?(bgrinstead) → review+
Comment on attachment 8726801 [details]
MozReview Request: Bug 1246088 - Safely access contentWindow in iframe and embed; r=bgrins

https://reviewboard.mozilla.org/r/38239/#review35627
Comment on attachment 8728371 [details]
MozReview Request: Bug 1246088 - ESLint cleanups of devtools/shared/layout/utils.js; r=bgrins

https://reviewboard.mozilla.org/r/38925/#review35629
Attachment #8728371 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/337116c1afac
https://hg.mozilla.org/mozilla-central/rev/36bd53d339e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
Attachment #8726801 - Flags: approval-mozilla-beta?
Attachment #8726801 - Flags: approval-mozilla-aurora?
arni2033, could you please verify this issue is fixed as expected in a latest Nightly build? Thanks!
Flags: needinfo?(arni2033)
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
Flags: needinfo?(arni2033)
Status: RESOLVED → VERIFIED
(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. :)
Comment on attachment 8726801 [details]
MozReview Request: Bug 1246088 - Safely access contentWindow in iframe and embed; r=bgrins

This was verified on Nightly, seems safe to uplift to Aurora47 and Beta46.
Attachment #8726801 - Flags: approval-mozilla-beta?
Attachment #8726801 - Flags: approval-mozilla-beta+
Attachment #8726801 - Flags: approval-mozilla-aurora?
Attachment #8726801 - Flags: approval-mozilla-aurora+
(In reply to Wes Kocher (:KWierso) from comment #28)
> Backed out from beta in
> https://hg.mozilla.org/releases/mozilla-beta/rev/2d3b3f657756 for dt bustage
> like:
> https://treeherder.mozilla.org/logviewer.html#?job_id=933079&repo=mozilla-
> beta
Turns out that Tilt (the devtools 3D view) relies on a function that the patch here renamed.
Tilt was removed in bug 1245875, so in FF47, which is why my change didn't break it in FF48 and FF47.
However, uplifting my patch to FF46 breaks devtools because Tilt is still present in that version.
I didn't anticipate this.
So, we can't uplift the fix to beta, unless we make another custom fix just for this version.
Flags: needinfo?(pbrosset)
Is this important enough to warrant making a custom fix? How hard to you think it would be to fix? The beta 6 build will be next week.  We could also wontfix this for 46.
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
Chris, do you think this is a must-fix for Beta46? We are wondering whether to build a custom patch for it because the aurora, nightly patch does not apply cleanly.
Flags: needinfo?(cpeterson)
I think we are ok to wontfix this rather than scrambling to fix the issue and do more uplifts in late beta. 
If anyone disagrees strongly, please let me know.
status-firefox46: affected → wontfix
(In reply to Ritu Kothari (:ritu) from comment #32)
> Chris, do you think this is a must-fix for Beta46?

No need to fix in Beta 46.
Flags: needinfo?(cpeterson)
Attachment #8726801 - Flags: approval-mozilla-beta+
This wasn't even needed for Beta 46, because youtube rewriting was disabled in bug 1255918
It took me a while to find that bug, yes.
status-firefox46: wontfix → unaffected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: