Bug 144095 - img.currentSrc problem in strict mode with old picturefill
Summary: img.currentSrc problem in strict mode with old picturefill
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-23 05:10 PDT by Simon Pieters
Modified: 2015-10-22 03:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (55.10 KB, patch)
2015-05-27 17:11 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Pieters 2015-04-23 05:10:50 PDT
See https://github.com/scottjehl/picturefill/issues/478

Microsoft discovered when implementing srcset in Project Spartan that some sites broke because they were using picturefill < 2.3.1 with "use strict" and picturefill was writing to currentSrc, which throws in strict mode. The issue has been fixed in picturefill but it's not certain all sites will upgrade in time. I think Microsoft decided to remove support for currentSrc for the time being. Alternative approaches would be to mark the property as [Replaceable] or have a no-op setter for now. Or leave it as is and evang any broken sites to upgrade picturefill.

If HTMLPictureElement is also supported, there is no issue with picturefill since it bails out early in that case. Safari 8 does not support currentSrc so this does not show up in current release.
Comment 1 Liam Quinn 2015-05-26 06:45:17 PDT
The EFL WebKit port has been affected by this for awhile now. With the fix for bug #144679, all ports would now be affected.

Most of the images do not load on http://www.espn.com because of this issue.
Comment 2 Yoav Weiss 2015-05-26 14:33:31 PDT
This is first and foremost a bug in the older versions of the polyfill.

With regard to ESPN, we have contacted their front-end engineers, and it seems like they will upgrade to the latest polyfill version shortly: https://twitter.com/steveclancy/status/603280160696225792

We (the RICG) also intend to send out multiple calls to the developer community to upgrade their polyfill versions immediately. I hope that would be enough to make this a non-issue.

Liam - Are there any other sites that you've encountered problems with? We can try to contact them directly as well.
Comment 3 Liam Quinn 2015-05-26 14:41:26 PDT
I have not yet encountered other sites affected by this. Thanks for reaching out to ESPN!
Comment 4 Dean Jackson 2015-05-27 17:11:26 PDT
Created attachment 253820 [details]
Patch
Comment 5 Dean Jackson 2015-05-27 17:12:52 PDT
This patch allows us to remove currentSrc easily. It doesn't actually change the built result by default.
Comment 6 Dean Jackson 2015-05-27 17:29:39 PDT
I landed an ENABLE guard in r184928. This can be removed once the real issue has been fixed.
Comment 7 Jon Lee 2015-05-29 00:50:31 PDT
Is anyone aware how long this has been going on? Is ESPN's use of the polyfill relatively recent? I'm slightly surprised it took this long to notice given that we've had this on for over a year.
Comment 8 Yoav Weiss 2015-06-01 01:06:11 PDT
Comment on attachment 253820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253820&action=review

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:119
> +#endif

There's no need for these HTMLPreloadScanner changes.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:161
> +#endif

Likewise, not needed in order to put currentSrc behind a flag