1306472 - No load event for iframe with javascript: URI that doesn't return a string

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: test case

This appears to be an unintentional regression from bug 1268047.  In Firefox 49 and later, the onload handler for this iframe is never executed:

    <iframe src="javascript:1" onload="alert('hello')"></iframe>

In other browsers, the onload handler *is* executed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm. So in 48, this testcase:

    <iframe src="javascript:undefined" onload="alert('hello')"></iframe>

doesn't fire the load event either, I guess?  And the only new issue is that we made `1` act like `undefined`?

Comment 2

[Matt Brubeck (:mbrubeck)]

The "javascript:undefined" testcase also changed behavior.  (It displays the alert in 48 but not in 49.)  So maybe I was wrong about the cause of the regression.

Comment 3

[Matt Brubeck (:mbrubeck)]

Sorry, comment #2 was wrong. (I messed up my test file, and was testing the wrong thing.)  Your guess was correct; "javascript:undefined" does not fire the load event in any version of Firefox, though it does in other browsers.

Comment 5

[Seth A. Roby]

Bug 1307283 was mine; just chiming in here now that my bug is a dupe of this one.

Note that this can be worked around by replacing the `src` with something like `about:blank`, which is the [fix][1] we used to make remotipart's file uploader work again, but it requires a code change on the server side, which no other browser cares about.

[1]: https://github.com/JangoSteve/remotipart/compare/v1.3.0...v1.3.1#diff-18f6f91c62fa429ea66ea59ceabcec7c

Comment 7

[Boris Zbarsky [:bzbarsky]]

Two things:

1)  Our new behavior is actually following the HTML spec, but clearly the spec is wrong here.  I filed https://github.com/whatwg/html/issues/1895 to get the spec changed.

2)  I strongly believe we should in fact fix this in 49, 50, 51.  In fact, I think we should back out bug 1268047, since the spec it was aligning with is not only clearly bogus but doesn't even match the browsers it was claiming to match (see <https://github.com/whatwg/html/pull/1107#issuecomment-253558923>).  Once the spec gets sorted out, we can see about aligning to whatever the new text is.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Back out bug 1268047, because the spec it tried to implement backs the web

Comment 9

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Breaks websites.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8800736 [details] [diff] [review]
Back out bug 1268047, because the spec it tried to implement backs the web

Approval Request Comment
[Feature/regressing bug #]: Bug 1268047
[User impact if declined]: Some sites are broken and are telling users to use
   other browsers.
[Describe test coverage new/current, TreeHerder]: This is backing out to our
   previous behavior, no new tests being added.
[Risks and why]: I think this is pretty low-risk, since it just restores the
   Firefox 48 behavior and we were not aware of any sites having issues with
   that behavior.  This is why I'm requesting 49 approval as well; this is
   causing real problems in the wild, and we should fix them ASAP.
[String/UUID change made/needed]: None.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5505bf10d2
Back out bug 1268047, because the spec it tried to implement backs the web.  r=smaug

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 50+ given the widespread impact.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800736 [details] [diff] [review]
Back out bug 1268047, because the spec it tried to implement backs the web

Given the severity, let's uplift this to Aurora51 and Beta50 promptly and stabilize in those channels. This will give us more confidence on the fix in case we need to do a 49 dot release.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Matt, once this lands on Nightly, could you please verify this issue is fixed as expected? Thanks!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/76da38551dce411d9d2d021853f91a2b63c1df42

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9126be480c45
followup.  Fix the timeout on the html/browsers/browsing-the-web/navigating-across-documents/014.html web platform test.  r=bustage

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/157eba9fe522

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking this as blocking 49 to indicate it is a driver for a 49.0.2 dot release.
 
It would be good to verify the fix in the beta 50.0b7 build released this Friday.
We could also land this on m-r now, and test from the treeherder builds.

Comment 19

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800736 [details] [diff] [review]
Back out bug 1268047, because the spec it tried to implement backs the web

Serious issue with 49, affecting several sites. Let's take this on mozilla-release for a 49.0.2 possibly for next week.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f5505bf10d2

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9126be480c45

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/21a1f994f8c0

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e6651e32d271d35974a5eca2e898e7c354b8ea4
https://hg.mozilla.org/releases/mozilla-beta/rev/ef78004ec7d3661c38a66b0cb2652533757fa911
https://hg.mozilla.org/releases/mozilla-release/rev/a18dedb8c8989f50a974e0036d4aa37b39499602

Comment 26

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/load-event-is-no-longer-fired-on-iframe-with-javascript-src-returning-non-string-value/

Comment 27

[Boris Zbarsky [:bzbarsky]]

Uh.. no.  That's not correct at all.  This bug is fixed, which means we _will_ fire that load event in 50+, and in 49 as well if we ship a dot-release after this point.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I mean, maybe that's what the entry is trying to say, but that's definitely not the impression it's creating!

Comment 29

[Kohei Yoshino [:kohei]]

I know what's happening here; this doc should actually have been written for Bug 1268047. Somehow it slipped under my radar. Modified the description as follows: https://github.com/fxsitecompat/www.fxsitecompat.com/commit/2080b0c

Comment 30

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

This also affects fennec, right?  I think this means we also need to release 49.0.2 for mobile.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> This also affects fennec, right?

That's correct.  :(

Comment 32

[Jukka Jylänki]

Hey, is it possible to add a regression test for this behavior somewhere so that Firefox won't regress in the future?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Yes.  I plan to add a web platform test once the spec gets sorted out so I know what the test should test.

If that takes more than about another week, I'll add a mochitest in the interim....

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Well, there were tests added when the regressing bug landed, since that made us follow the spec, but the spec was wrong, and also those tests.

I would expect us to get wpt tests for this once the spec issue is sorted out.
Right now it is still unclear what the behavior actually should be - clearly not what the spec says, but what then, dunno.

See
https://github.com/whatwg/html/issues/1895
https://github.com/whatwg/html/issues/1896

Comment 35

[Matt Brubeck (:mbrubeck)]

Verified fixed in the latest Nightly.

Comment 36

[Andrei Vaida [:avaida]]

Updating flags based on Comment 35. We'll make sure this gets verified on 49.0.2 and 50.0b8 as well. ni?myself as a reminder.

Comment 37

[Andrei Vaida [:avaida]]

I've managed to reproduce this bug on an affected build (49.0.1-build3, 20160922113459).

This is verified fixed on:

  - 49.0.2-build1 (20161018030522)
  - 50.0b8-build1 (20161017130949)
  - 51.0a2 (2016-10-18)

using Windows 10 x64, Ubuntu 14.04 x86 and Mac OS X 10.11.6. The onload event is fired, as expected.

Comment 38

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Release Note Request (optional, but appreciated)
[Why is this notable]: driver for dot release
[Suggested wording]:
[Links (documentation, blog post, etc)]: Web compatibility issue with file uploads (Bug 1306472)

I'm happy to change the release note wording if someone can come up with a suggestion.

Comment 39

[Mary D. Emerson]

-- System Details --
Details:
        Minecraft Version: 1.7.10
        Operating System: Windows XP (x86) version 5.1
        Java Version: 1.8.0_25, Oracle Corporation
        Java VM Version: Java HotSpot(TM) Client VM (mixed mode), Oracle Corporation
        Memory: 141971272 bytes (135 MB) / 292806656 bytes (279 MB) up to 523501568 bytes (499 MB)
        JVM Flags: 6 total; -XX:HeapDumpPath=MojangTricksIntelDriversForPerformance_javaw.exe_minecraft.exe.heapdump -Xmx512M -XX:+UseConcMarkSweepGC -XX:+CMSIncrementalMode -XX:-UseAdaptiveSizePolicy -Xmn128M
        AABB Pool Size: 0 (0 bytes; 0 MB) allocated, 0 (0 bytes; 0 MB) used
        IntCache: cache: 0, tcache: 0, allocated: 0, tallocated: 0
        FML: MCP v9.05 FML v7.10.85.1291 Minecraft Forge 10.13.2.1291 36 mods loaded, 36 mods active
        mcp{9.05} [Minecraft Coder Pack] (http://unblockedallgames.com/unblocked-games/minecraft.jar) Unloaded->Constructed->Pre-initialized
        FML{7.10.85.1291} [Forge Mod Loader] (forge-1.7.10-10.13.2.1291.jar) Unloaded->Constructed->Pre-initialized
        Forge{10.13.2.1291} [Minecraft Forge] (forge-1.7.10-10.13.2.1291.jar) Unloaded->Constructed->Pre-initialized
        CodeChickenCore{1.0.4.29} [CodeChicken Core] (minecraft.jar) Unloaded->Constructed->Pre-initialized
        Micdoodlecore{} [Micdoodle8 Core] (minecraft.jar) Unloaded->Constructed->Pre-initialized
        NotEnoughItems{1.0.3.74} [Not Enough Items] (NotEnoughItems-1.7.10-1.0.3.74-universal.jar) Unloaded->Constructed->Pre-initialized

Comment 40

[nikolabelaa]

Any solution? same thing happens to my while trying to access https://www.unblockedgamesroom.com/ at my school.

Comment 41

[Boris Zbarsky [:bzbarsky]]

This bug is long-fixed.  What makes you think this is the problem you're seeing?

Comment 42

[Boris Zbarsky [:bzbarsky]]

Bug 1477821 tracks updating to the spec once that's sorted out.