window.location.replace('#anchor') FAIL #1584

Closed
CoolCmd opened this Issue Jul 7, 2012 · 7 comments

Projects

None yet

3 participants

@CoolCmd
CoolCmd commented Jul 7, 2012

Code:

window.location.replace('#anchor');

Result:
All browsers and Firefox13+Scriptish 0.1.7 produces URL http://sample.com/#anchor
Firefox13+GM 0.9.20 produces URL chrome://browser/content/browser.xul#anchor

@Ventero
Contributor
Ventero commented Jul 7, 2012

Here's a simple testcase: https://gist.github.com/3066765
Note that with GM HEAD, this requires a @grant directive to make sure the script runs inside a sandbox.

It's also interesting to note that location.assign works without any problems, which in my opinion points to an upstream bug.

@arantius
Collaborator

Confirmed. But I agree, it seems upstream. I don't know of anything we're doing that could cause this. But if Scriptish works, it kinda means we are doing something wrong.

@Ventero
Contributor
Ventero commented Jul 16, 2012

The reason this works in Scriptish is because they use an content-document-global-created observer to inject the scripts as opposed to Greasemonkey's DOMContentLoaded listener. When applying the following (hackish) patch to GM HEAD, the testcase works just fine: https://gist.github.com/3fa78dc4278eed0a6bb9
This shows that the problem is definitely an upstream bug.

@arantius
Collaborator

Interesting. Feels like a broken side-effect workaround rather than a fix.

But most importantly, what does "Sent immediately after a web content document window has been set up, but before any script code has been executed." actually mean? That doesn't sound like DOMContentLoaded very much.

@Ventero
Contributor
Ventero commented Jul 16, 2012

Actually what I said in my earlier comment isn't quite correct. Scriptish uses the content-document-global-created notification to attach a DOMContentLoaded listener to the content window directly, instead of attaching it to appcontent. That way they still ensure the scripts are run after DOMContentLoaded fired, but as a side effect they're not hitting this bug. "Patch" (as you said, this isn't a real fix, it's just to show that the difference is indeed in attaching the event listener to the window directly) for GM to show this behavior: https://gist.github.com/3125215

@arantius
Collaborator

That way they still ensure the scripts are run after DOMContentLoaded fired, but as a side effect they're not hitting this bug.

Do you know why this works around the bug?

@Ventero
Contributor
Ventero commented Aug 5, 2012

Do you know why this works around the bug?

I'm pretty sure it's just coincidence. I've created a minimal addon to show that this is indeed an upstream bug:
http://ventero.de/temp/locationreplacetest.xpi

This addon does two things:

  • It creates an XPCOM service, which listens for content-document-global-created notifications, and upon receiving one ẁith an href of http://ventero.de/#location, it calls location.replace("#foo") on the corresponding content window.
  • It also adds an DOMContentLoaded event listener to appcontent, which when receiving an event for http://ventero.de/#location2 calls location.replace("#bar").

As expected, when visiting #location, everything works (i.e. the final page loaded is http://ventero.de/#foo). When visiting #location2 however, the final page is chrome://browser/content/browser.xul#bar.

This should prove that the problem is indeed an upstream bug and not caused by Greasemonkey. The fact that Scriptish is not affected by this bug is simply because the bug only appears when calling location.replace in response to a DOMContentLoaded event on appcontent.

@arantius arantius added a commit that closed this issue Dec 20, 2012
@arantius arantius Tweak the injection logic to work around bugs.
1) Attach event listener to content windows (not appcontent element), so location.replace('#anything') works.
2) Also attach a load (as well as DOMContentLoaded) listener, for non-DOM loads like images.

Fixes #1584
Refs #1675
27e9b73
@arantius arantius closed this in 27e9b73 Dec 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment