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)

56 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ verified
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: jeclark, Assigned: qdot)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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
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
Repro'd as stated in Comment 0 on windows and linux. Will take a look.
Assignee: nobody → kyle
Flags: needinfo?(kyle)
Priority: -- → P2
Have encountered this in the 59 branch as well.  Will post bug report now.
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 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?
Flags: needinfo?(kyle)
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="&amp;src=https://stream.rotaxowner.com/vod/loctite.f4m&amp;autoHideControlBar=true&amp;streamType=vod&amp;autoPlay=true&amp;verbose=true">
  <embed allowfullscreen="true" bgcolor="#000000" flashvars="&amp;src=https://stream.rotaxowner.com/vod/loctite.f4m&amp;autoHideControlBar=true&amp;streamType=vod&amp;autoPlay=true&amp;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)
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 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+
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).
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66a5836c66b1
Revert changes to classid checks for object tags; r=bz
Attachment #8972240 - Flags: approval-mozilla-beta?
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
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
https://hg.mozilla.org/mozilla-central/rev/66a5836c66b1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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-
Oh, ok, I thought I needed approval for 61 uplift on this, didn't mean it to be 60.
Flags: qe-verify+
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+
Apparently this fix caused another regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1279218#c71
Flags: needinfo?(kyle)
That page has an <object classid> on it.  It shouldn't play.

The real question is why it plays in 60....
Oh, because this bug is present in 60!

Does that page play in 55?
Sergiu, please see comment #23.
Flags: needinfo?(kyle) → needinfo?(sergiu.logigan)
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.
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...
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)
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.
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.