Closed Bug 1306472 Opened 8 years ago Closed 8 years ago

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

Categories

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

49 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox49 blocking verified
relnote-firefox --- 49+
firefox50 + verified
firefox51 + verified
firefox52 + verified

People

(Reporter: mbrubeck, Assigned: bzbarsky)

References

Details

(4 keywords)

Attachments

(2 files)

Attached file 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)
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)
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.
Flags: needinfo?(bzbarsky)
Assignee: nobody → afarre
Blocks: 1308133
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
Blocks: 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)
Assignee: afarre → bzbarsky
Status: NEW → ASSIGNED
[Tracking Requested - why for this release]: Breaks websites.
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+
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
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)
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
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 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+
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/0f5505bf10d2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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)
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
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.
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.
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: --- → ?
No longer blocks: 1307420, 1308133
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: