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

VERIFIED FIXED in Firefox 49

Status

()

VERIFIED FIXED
2 years ago
7 months ago

People

(Reporter: mbrubeck, Assigned: bzbarsky)

Tracking

(4 keywords)

49 Branch
mozilla52
compat, dev-doc-complete, regression, site-compat
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox49blocking verified, relnote-firefox 49+, firefox50+ verified, firefox51+ verified, firefox52+ verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8796314 [details]
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.
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`?
Flags: needinfo?(mbrubeck)
(Reporter)

Comment 2

2 years ago
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.
Flags: needinfo?(mbrubeck)
(Reporter)

Comment 3

2 years ago
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.
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
Flags: needinfo?(bzbarsky)
Assignee: nobody → afarre
Blocks: 1308133
Duplicate of this bug: 1307283

Comment 5

2 years ago
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

Updated

2 years ago
Blocks: 1307420
status-firefox49: affected → wontfix
Duplicate of this bug: 1307420
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.
Flags: needinfo?(bzbarsky)
Created attachment 8800736 [details] [diff] [review]
Back out bug 1268047, because the spec it tried to implement backs the web
Attachment #8800736 - Flags: review?(bugs)
Assignee: afarre → bzbarsky
Status: NEW → ASSIGNED
[Tracking Requested - why for this release]: Breaks websites.
status-firefox49: wontfix → affected
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
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.
Attachment #8800736 - Flags: approval-mozilla-release?
Attachment #8800736 - Flags: approval-mozilla-beta?
Attachment #8800736 - Flags: approval-mozilla-aurora?
Attachment #8800736 - Flags: review?(bugs) → review+

Comment 11

2 years ago
Pushed by bzbarsky@mozilla.com:
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
Tracked for 50+ given the widespread impact.
tracking-firefox50: ? → +
tracking-firefox51: ? → +
tracking-firefox52: ? → +
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.
Attachment #8800736 - Flags: approval-mozilla-beta?
Attachment #8800736 - Flags: approval-mozilla-beta+
Attachment #8800736 - Flags: approval-mozilla-aurora?
Attachment #8800736 - Flags: approval-mozilla-aurora+
Hello Matt, once this lands on Nightly, could you please verify this issue is fixed as expected? Thanks!
Flags: needinfo?(mbrubeck)

Comment 16

2 years ago
Pushed by bzbarsky@mozilla.com:
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

2 years ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/157eba9fe522
status-firefox51: affected → fixed
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.
tracking-firefox49: ? → blocking
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.
Attachment #8800736 - Flags: approval-mozilla-release? → approval-mozilla-release+
Keywords: dev-doc-needed, site-compat
Flags: qe-verify+

Comment 20

2 years ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/0f5505bf10d2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Duplicate of this bug: 1308133
Duplicate of this bug: 1310414
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.
Flags: needinfo?(kohei.yoshino)
I mean, maybe that's what the entry is trying to say, but that's definitely not the impression it's creating!
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
Flags: needinfo?(kohei.yoshino)
This also affects fennec, right?  I think this means we also need to release 49.0.2 for mobile.
Flags: needinfo?(bzbarsky)
> This also affects fennec, right?

That's correct.  :(
Flags: needinfo?(bzbarsky)

Comment 32

2 years ago
Hey, is it possible to add a regression test for this behavior somewhere so that Firefox won't regress in the future?
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....
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
(Reporter)

Comment 35

2 years ago
Verified fixed in the latest Nightly.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mbrubeck)
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.
status-firefox52: fixed → verified
Flags: needinfo?(andrei.vaida)
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.
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox51: fixed → verified
Flags: needinfo?(andrei.vaida)
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.
relnote-firefox: --- → ?
relnote-firefox: ? → 49+

Updated

2 years ago
No longer blocks: 1307420, 1308133
Comment hidden (spam)
Comment hidden (spam)
This bug is long-fixed.  What makes you think this is the problem you're seeing?
Flags: needinfo?(nikolabelaa)
Flags: needinfo?(nikolabelaa)
Bug 1477821 tracks updating to the spec once that's sorted out.
You need to log in before you can comment on or make changes to this bug.