Bug 75369

Summary:	Change in location href format for frames with applewebdata: location
Product:	WebKit	Reporter:	Daniel Jalkut <jalkut>
Component:	DOM	Assignee:	Nobody <webkit-unassigned>
Status:	NEW ---
Severity:	Normal	CC:	ap, arv, beidson, darin
Priority:	P1	Keywords:	InRadar
Version:	528+ (Nightly build)
Hardware:	Mac
OS:	All

Description Daniel Jalkut 2011-12-29 14:06:11 PST

WebKit nightly builds return a different format href for applewebdata: based frames than shipping versions of Safari.

I believe the format change occurred with changes in:

https://bugs.webkit.org/show_bug.cgi?id=30225

Notice how the WorkerLocation and Location "href" methods uses to append a "/" to the end of the string if there is no path? That no longer happens.

A concrete example of the bug occurs when you load a web view with a nil baseURL:


	NSString* testDocument = @"\
	<html>\
	<head>\
	<script>document.write(\"hello - our window location href is \" + window.location.href + \",<br /> document location href is \" + document.location.href);</script>\
	</head>\
	<body>\
	</body>\
	</html>";
	
	[[[self webView] mainFrame] loadHTMLString:testDocument baseURL:nil];

If you run this code on trunk builds of WebKit you'll see a location href like:

applewebdata://A7E269AE-63E7-468C-956A-6A5F670F99DF

While on shipping versions of WebKit in Safari for Mac, you'll see a location href like:

applewebdata://A7E269AE-63E7-468C-956A-6A5F670F99DF/

There is a real-world consequence of this that I ran into: when using TinyMCE in a WebView that has a nil baseURL, TinyMCE uses a regular expression to derive the base URL. It turns out that regular expression depends upon the trailing slash in applewebdata: URLs, or an invalid URL is stored and causes exceptions in the loading of the TinyMCE framework.

Note that I don't think the "canonicalize" URL tests cover this situation because the canonicalize method uses the href attribute of an anchor and doesn't exercise the location code.

This could probably also be effectively addressed by taking care to included a closing / in the unique applewebdata: URL generator. This stems from the fact that the baseURI for the page is in fact missing a trailing slash, but previously it was being added in during the location href lookup.

Comment 1 Erik Arvidsson 2011-12-29 14:42:09 PST

I'm currently on vacation... Is applewebdata hierarchical or not? If it is then we should include a trailing slash. If not then we probably only need to update isHierarchical. If it is hierarchical and we still want to strip the trailing slash this scheme has to be special cased.

Comment 2 Darin Adler 2012-01-08 22:59:10 PST

The applewebdata scheme is not hierarchical.

If we want to add the slash for backwards-compatibility, I believe we can modify the createUniqueWebDataURL() function in WebFrame.mm and we probably won't have to make any other changes.

Comment 3 Daniel Jalkut 2012-01-09 06:41:38 PST

(In reply to comment #2)
> The applewebdata scheme is not hierarchical.
> 
> If we want to add the slash for backwards-compatibility, I believe we can modify the createUniqueWebDataURL() function in WebFrame.mm and we probably won't have to make any other changes.

How widespread are these web data URLs used? The "createUniqueWebDataURL" has been creating the URLs without the "/" appended, and the slashes (as far as I can tell) have been getting added only in the context of resolving location.href. So if there are other, non-location DOM nodes that contain the URLs, then adding the slash at creation might create a different kind of incompatibility.

My personal investment in this bug is over since I added a workaround in my copy of TinyMCE to avoid the issue I was seeing. Maybe it won't turn out to be a major issue for people, but I wanted to raise it so you can consider the implications.

Comment 4 Alexey Proskuryakov 2012-01-09 08:47:10 PST

<rdar://problem/10662839>

Comment 5 Darin Adler 2012-01-09 13:49:30 PST

(In reply to comment #3)
> How widespread are these web data URLs used?

The URL is used internally in any Mac application that uses the loadData method, a way to load a webpage from a passed in string or data object. Basically not used in the web browser at all.

> The "createUniqueWebDataURL" has been creating the URLs without the "/" appended, and the slashes (as far as I can tell) have been getting added only in the context of resolving location.href. So if there are other, non-location DOM nodes that contain the URLs, then adding the slash at creation might create a different kind of incompatibility.

That does seem possible.

> My personal investment in this bug is over since I added a workaround in my copy of TinyMCE to avoid the issue I was seeing. Maybe it won't turn out to be a major issue for people, but I wanted to raise it so you can consider the implications.

I see. There may well be nobody else affected.

Comment 6 Timothy Hatcher 2012-01-09 13:59:32 PST

Does this affect a shipping version of MarsEdit?

Comment 7 Daniel Jalkut 2012-02-17 05:22:17 PST

(In reply to comment #6)
> Does this affect a shipping version of MarsEdit?

Sorry, I missed this comment until just now when I came back to check on this after users started reporting issues with MarsEdit after installing Safari 5.2 beta.

The bug does affect the latest shipping version of MarsEdit: 

http://red-sweater.com/marsedit/

I have a workaround checked in but haven't released a new version yet.

Comment 8 Daniel Jalkut 2012-02-17 05:22:48 PST

To clarify, and for the record, it affects MarsEdit 3.4.2.

Comment 9 Brady Eidson 2012-02-17 09:56:04 PST

As Darin mentioned, applewebdata: urls are not hierarchical.

It's unfortunate that fixing our bug that added the trailing slash broke a few apps, but it still was a bug that was fixed.

I see two obvious courses of action:
1 - Do nothing, as it appears TinyMCE and MarsEdit have things under control in upcoming releases
2 - Change the behavior back with a LinkedOnOrAfter quirk.  That way, older versions of applications would get the old behavior, but any time an application was built and linked against a newer version of WebKit that has had the bug fixed it'd be expected that they adopt the new behavior.

Comment 10 Daniel Jalkut 2012-02-17 14:20:56 PST

(In reply to comment #9)
> I see two obvious courses of action:
> 1 - Do nothing, as it appears TinyMCE and MarsEdit have things under control in upcoming releases
> 2 - Change the behavior back with a LinkedOnOrAfter quirk.  That way, older versions of applications would get the old behavior, but any time an application was built and linked against a newer version of WebKit that has had the bug fixed it'd be expected that they adopt the new behavior.

I appreciate that it was a bug fix and if the new behavior is correct I'm happy to see it stay that way going forward.

I realize there is a non-zero engineering and maintenance cost to adding back the functionality with a  LinkedOnOrAfter test, but I would really appreciate it if you could do this because:

1. Many of my customers will not update my software promptly, and will contact me for support after seeing the failures in the app. Already with the beta I see there is a failure to connect the failure in my app with the update to Safari. "I did recently update to Safari 5.2 beta, but I don't see how that could possibly affect this."

2. Maintaining functionality of my previous releases makes it easier for me to do regression testing of my stuff as I'm debugging and enhancing the WebKit based rich editor in the app. If all my releases up to now stop working in WebKit going forward, it will just make that testing a bit harder.

3. Any other developers who are not tuned-in and may be hurt by this change would be spared their own version of this pain … without any effort on their part! You'll rest easier ;)

Thanks for considering it,
Daniel

Comment 11 Brady Eidson 2012-02-17 14:38:43 PST

(In reply to comment #10)
> 2. Maintaining functionality of my previous releases makes it easier for me to do regression testing of my stuff as I'm debugging and enhancing the WebKit based rich editor in the app. If all my releases up to now stop working in WebKit going forward, it will just make that testing a bit harder.

To be very clear, the workaround we would do would break your maintenance of previous releases if you were rebuilding them from source.  This is intentional; Whenever we make this type of change, we force developers to update to the new behavior when they rebuild their source.

Comment 12 Daniel Jalkut 2012-02-17 14:48:20 PST

(In reply to comment #11)
> (In reply to comment #10)
> > 2. Maintaining functionality of my previous releases makes it easier for me to do regression testing of my stuff as I'm debugging and enhancing the WebKit based rich editor in the app. If all my releases up to now stop working in WebKit going forward, it will just make that testing a bit harder.
> 
> To be very clear, the workaround we would do would break your maintenance of previous releases if you were rebuilding them from source.  This is intentional; Whenever we make this type of change, we force developers to update to the new behavior when they rebuild their source.

Yep, I understand, and have worked with similar link-version fixes to e.g. AppKit bugs.

I usually run archived copies of my apps when testing old behavior, specifically to avoid possibility of compile-time behavior changes from e.g. checking out and rebuilding the app at an old revision.