Closed
Bug 1433221
Opened 6 years ago
Closed 6 years ago
Firefox does not send NPP_NewStream() to Flash Player when constructing player instance on some content
Categories
(Core :: DOM: Core & HTML, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: jeclark, Assigned: qdot)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
jcristau
:
approval-mozilla-beta-
jcristau
:
approval-mozilla-esr60+
|
Details |
Method: Go Here: https://www.rotax-owner.com/en/videos-topmenu/free-videos/457-loctite-rotax Play the video Hit Refresh in the browser Observe that the video doesn't load Right-Click on the area where the video should be Observe that Flash Player says Movie Not Loaded Workaround: Clear the browser history and refresh the page Developer Analysis: I am confident this is a browser problem – we are receiving the NPP_New() when the player instance is constructed, but not the NPP_NewStream(). For the NPAPI, we do not request the root movie, the browser constructs a stream on our behalf from the url in the embed/object element. Regression Range: 11:38.91 INFO: Narrowed nightly regression window from [2017-07-28, 2017-07-31] (3 days) to [2017-07-30, 2017-07-31] (1 days) (~0 steps left) 11:38.91 INFO: Got as far as we can go bisecting nightlies... 19:03.87 INFO: Using local file: /var/folders/td/0dxh37fn78v51w48tk73w41m0000gn/T/tmpF7SiKZ/2ee0b5a0560a-autoland-target.dmg (downloaded in background) 19:03.87 INFO: Running autoland build built on 2017-07-30 19:15:10.327000, revision 2ee0b5a0 19:43.24 INFO: Launching /private/var/folders/td/0dxh37fn78v51w48tk73w41m0000gn/T/tmptrmMls/Nightly.app/Contents/MacOS/firefox 19:43.24 INFO: Application command: /private/var/folders/td/0dxh37fn78v51w48tk73w41m0000gn/T/tmptrmMls/Nightly.app/Contents/MacOS/firefox -foreground -profile /var/folders/td/0dxh37fn78v51w48tk73w41m0000gn/T/tmpmVmSft.mozrunner 19:47.52 INFO: application_buildid: 20170730183658 19:47.52 INFO: application_changeset: 2ee0b5a0560a0c9f6dfd023dfe8a26c738994bb9 19:47.52 INFO: application_name: Firefox 19:47.52 INFO: application_repository: https://hg.mozilla.org/integration/autoland 19:47.52 INFO: application_version: 56.0a1 Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): bad 20:22.47 INFO: Narrowed inbound regression window from [00af61b0, 5c6a8f30] (3 builds) to [00af61b0, 2ee0b5a0] (2 builds) (~1 steps left) 20:22.47 INFO: No more inbound revisions, bisection finished. 20:22.47 INFO: Last good revision: 00af61b0dd2ff664ecc4b23d668219505930ee92 20:22.47 INFO: First bad revision: 2ee0b5a0560a0c9f6dfd023dfe8a26c738994bb9 20:22.47 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=00af61b0dd2ff664ecc4b23d668219505930ee92&tochange=2ee0b5a0560a0c9f6dfd023dfe8a26c738994bb9
Comment 1•6 years ago
|
||
Thanks, Jeromie for your support and for performing a regression range. Hello Kyle, from the pushlog it stands out the fact that changes from bug 1279218 are related to this bug, can you please take a look? Thanks
Component: Untriaged → DOM
Flags: needinfo?(kyle)
Product: Firefox → Core
Assignee | ||
Comment 2•6 years ago
|
||
Repro'd as stated in Comment 0 on windows and linux. Will take a look.
Assignee: nobody → kyle
Flags: needinfo?(kyle)
Updated•6 years ago
|
Priority: -- → P2
Comment 3•6 years ago
|
||
Have encountered this in the 59 branch as well. Will post bug report now.
Updated•6 years ago
|
Keywords: regression
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
Problem here was that clsid checking was removed with applets, as it looked like it wasn't really used other than java. Turns out, jquery uses it for identifying flash object tags (https://stackoverflow.com/questions/7941527/what-is-the-relevance-of-clsidd27cdb6e-ae6d-11cf-96b8-444553540000), and setting up the ObjectLoadingContent type correctly depends on it. This patch restores the minimum code needed to get that working again.
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8972240 [details] Bug 1433221 - Revert changes to classid checks for object tags; https://reviewboard.mozilla.org/r/240908/#review246876 ::: dom/base/nsObjectLoadingContent.cpp:1589 (Diff revision 1) > + thisElement->GetAttr(kNameSpaceID_None, nsGkAtoms::classid, classIDAttr); > + if (!classIDAttr.IsEmpty()) { > + // XXX(johns): Our de-facto behavior since forever was to refuse to load > + // Objects who don't have a classid we support, regardless of other type > + // or uri info leads to a valid plugin. > + newMime.Truncate(); So is this truncation the thing that fixes the bug? What is the actual markup involved here?
Updated•6 years ago
|
Flags: needinfo?(kyle)
Assignee | ||
Comment 7•6 years ago
|
||
Ok, on further inspection this seems to have nothing to do with the mime truncation, and the class id itself may be a misnomer too. I think this is about state invalidation and fallbacks. Here's the original block from the website in Comment #0: <object classid="clsid:d27cdb6e-ae6d-11cf-96b8-444553540000" id="SampleMediaPlayback" name="SampleMediaPlayback" type="application/x-shockwave-flash" width="680" height="383"> <param name="movie" value="https://stream.rotaxowner.com/swfs/SampleMediaPlayback.swf"> <param name="quality" value="high"> <param name="bgcolor" value="#f3f3f3"> <param name="allowfullscreen" value="true"> <param name="flashvars" value="&src=https://stream.rotaxowner.com/vod/loctite.f4m&autoHideControlBar=true&streamType=vod&autoPlay=true&verbose=true"> <embed allowfullscreen="true" bgcolor="#000000" flashvars="&src=https://stream.rotaxowner.com/vod/loctite.f4m&autoHideControlBar=true&streamType=vod&autoPlay=true&verbose=true" id="SampleMediaPlayback" name="SampleMediaPlayback" pluginspage="http://www.adobe.com/go/getflashplayer" quality="high" src="https://stream.rotaxowner.com/swfs/SampleMediaPlayback.swf" type="application/x-shockwave-flash" width="680" height="383"> </object> With the original applet patch, I removed classid checking, which did 2 things: - Truncate MIME type - Set state to invalid The truncation of MIME type had no bearing on this and was redundant anyways, as that would happen in the state invalidation block later too (https://searchfox.org/mozilla-central/source/dom/base/nsObjectLoadingContent.cpp#1795). However, since the object tag in the above example has a classid, if we invalidate state on the presence of classid, we invalidate the object tag and look at the child tags. In this case, the embed tag works. If I pull the embed node of the object node and place it as a child of the enclosing div, the movie comes up in nightly (v61). When I removed classid checking as part of applet tag removal, the state invalidation no longer happened, meaning for some reason we handled things correctly on the first load, but after first load, the object tag was seen as valid, but since it doesn't have the flash variables in either its own attributes and we apparently ignore the param tags, the movie doesn't load. Things I'm still not sure of and need to research more: - Why the first load works but subsequent loads don't/Why we don't always fall back to the embed tag - Why we aren't parsing param tags (they show up as disabled in devtools inspector, at least) - What the HTML spec says we're supposed to do in this case Basically just need to figure out what behavior conforms to spec expectations.
Flags: needinfo?(kyle)
Comment 8•6 years ago
|
||
Oh, yeah. The "keep ignoring <object> with classid" code needed to stay. I'm sorry I missed that. In terms of the spec, the spec says, at <https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-object-element> and then scrolling a ways down, in the "steps to (re)determine what the object element represents": If the classid attribute is present, and has a value that isn't the empty string, then: if the user agent can find a plugin suitable according to the value of the classid attribute, and either plugins aren't being sandboxed or that plugin can be secured, then that plugin should be used, and the value of the data attribute, if any, should be passed to the plugin. If no suitable plugin can be found, or if the plugin reports an error, jump to the step below labeled fallback. In the case of Firefox, we can never find a value suitable according to the value (the clsid thing is for ActiveX plugins) and hence should fallback. I'm really surprised we didn't have a test for this... Or is that test passing for the same reason that the original page passed on first load or something?
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8972240 [details] Bug 1433221 - Revert changes to classid checks for object tags; https://reviewboard.mozilla.org/r/240908/#review247114 ::: commit-message-13f68:5 (Diff revision 1) > +Bug 1433221 - Revert changes to classid checks for object tags; r?bz > + > +As part of applet/java plugin removal in bug 1279218, classid checking > +was also removed, as it was not obvious that this was used anywhere > +other than java plugins. However, jquery uses clsid to identify flash The commit message needs changes to explain that we want to keep ignoring <object> with an ActiveX classid. ::: dom/base/nsObjectLoadingContent.h:306 (Diff revision 1) > eSupportImages = 1u << 0, // Images are supported (imgILoader) > eSupportPlugins = 1u << 1, // Plugins are supported (nsIPluginHost) > eSupportDocuments = 1u << 2, // Documents are supported > // (nsIDocumentLoaderFactory) > // This flag always includes SVG > - eSupportClassID = 1u << 3, // The classid attribute is supported. No > + eSupportClassID = 1u << 3, // The classid attribute is supported. Used with This should really be eFallbackIfClassIDPresent or something. And it's not used with anything, as you discovered....
Attachment #8972240 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•6 years ago
|
||
I'm not seeing anything that specifically tests this, so I'll file a followup for that. I want to get this landed ASAP since we're breaking content (though I think I'm probably WAY too late for 60 uplift).
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
Pushed by kmachulis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/66a5836c66b1 Revert changes to classid checks for object tags; r=bz
Assignee | ||
Updated•6 years ago
|
Attachment #8972240 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 14•6 years ago
|
||
Ah, the fun of having switched window managers with new keybinds. That bug/feature causing regression was supposed to be https://bugzilla.mozilla.org/show_bug.cgi?id=1279218
Assignee | ||
Comment 15•6 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: https://bugzilla.mozilla.org/show_bug.cgi?id=1279218 [User impact if declined]: Some sites using flash with tags involving classid attributes may fail [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: By developer, still needs QA verification [Needs manual test from QE? If yes, steps to reproduce]: See Comment 0 [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Reestablishes a check that was already in the browser pre-56 [String changes made/needed]: None
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/66a5836c66b1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Blocks: 1279218
status-firefox59:
--- → wontfix
status-firefox60:
--- → wontfix
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → affected
No longer depends on: 1279218
Comment 17•6 years ago
|
||
Comment on attachment 8972240 [details] Bug 1433221 - Revert changes to classid checks for object tags; I'm going to pass for 60.0 (we're at RC2, this is not new in 60, and only just made it to nightly), but leave the uplift request for 60.1esr.
Attachment #8972240 -
Flags: approval-mozilla-esr60?
Attachment #8972240 -
Flags: approval-mozilla-beta?
Attachment #8972240 -
Flags: approval-mozilla-beta-
Assignee | ||
Comment 18•6 years ago
|
||
Oh, ok, I thought I needed approval for 61 uplift on this, didn't mean it to be 60.
Updated•6 years ago
|
tracking-firefox-esr60:
--- → 61+
Flags: qe-verify+
Comment 19•6 years ago
|
||
Comment on attachment 8972240 [details] Bug 1433221 - Revert changes to classid checks for object tags; fix a regression on some flash sites, approved for 60.1esr
Attachment #8972240 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 20•6 years ago
|
||
Apparently this fix caused another regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1279218#c71
Flags: needinfo?(kyle)
Comment 21•6 years ago
|
||
That page has an <object classid> on it. It shouldn't play. The real question is why it plays in 60....
Comment 22•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/905a6312336f
Comment 23•6 years ago
|
||
Oh, because this bug is present in 60! Does that page play in 55?
Comment 24•6 years ago
|
||
Sergiu, please see comment #23.
Flags: needinfo?(kyle) → needinfo?(sergiu.logigan)
Comment 25•6 years ago
|
||
The answer is yes, it does. But it sends different markup to 55 and 56. The 55 markup includes an <embed> inside the object; for 56 and later it does not. I haven't done any testing to see _why_ it sends different markup; whether it's UA sniffing or object-sniffing of some sort.
Comment 26•6 years ago
|
||
Per https://github.com/webcompat/web-bugs/issues/17004#issuecomment-392608211 the site mentioned in comment 20 does version and browser sniffing and sends broken markup (clsid without fallback) to only Firefox and only versions newer than 55. That markup used to work due to this bug, but once we fixed this bug it stopped working...
Comment 27•6 years ago
|
||
The site owner is about to be contacted, in order to fix this issue. https://github.com/webcompat/web-bugs/issues/17004#issuecomment-392664539
Flags: needinfo?(sergiu.logigan)
Comment 28•6 years ago
|
||
I retested this issue On FF beta 61.0b9 , nightly 61 as well as version 62.0a1 (2018-05-30) and I can't reproduce it on any of these versions, I can however reproduce this issue in version 60.0esr as well as 60.0.1esr. I will update the flags accordingly.
status-firefox62:
--- → verified
Comment 29•6 years ago
|
||
I have retested this issue on FF esr60.0.2 using Linux and Windows 10 and i can no longer reproduce it, i will mark it as verified.
Status: RESOLVED → VERIFIED
Comment 30•6 years ago
|
||
Please note i used this build to verify this issue : https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&fromchange=efbcc205a0d3562691bf62bb0cb55bb7e891e8ff&selectedJob=182023820
Flags: qe-verify+
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•