Frame inspection needs to die for a number of reasons. It's not really IonMonkey-safe, and it goes against our new security model. There are only a handful of uses of this stuff left, so let's kill it.

These two functions do the exact same thing, as far as I can tell.

Per an IRC discussion with jlebar, this is what we're actually intending to do here. Justin's original reasoning for this check is in bug 593174 comment 25.

Now with less voodoo!

\o/

> Part 2 - Fix incorrect test. v1 Can you check what the spec says and what other browsers do in this instance? This is a web-visible change of behavior, iiuc.

(In reply to Justin Lebar [:jlebar] from comment #7) > Can you check what the spec says and what other browsers do in this > instance? This is a web-visible change of behavior, iiuc. I believe the change here is spec correct, since the referrer is supposed to come off the script entry point, and the script entry point gets set for event handlers. Let me work up a browser-neutral testcase.

Comment on attachment 667378 [details] [diff] [review] Part 2 - Compare the document URI to the document principal, not the subject principal. v1 I've racked my brain and I /think/ this is right, but I'm not at all confident. I'd feel a lot better if Boris had a look.

Comment on attachment 667380 [details] [diff] [review] Part 4 - Remove API. v1 r=me assuming the rest of the patches shake out.

Comment on attachment 667377 [details] [diff] [review] Part 2 - Fix incorrect test. v1 r=me assuming that this matches behavior in other browsers.

Comment on attachment 667378 [details] [diff] [review] Part 2 - Compare the document URI to the document principal, not the subject principal. v1 I guess this is fine, though this whole heuristic is kinda questionable in various ways. If we're happy looking at the _document_ principal, why not just go ahead and use the current URI of the document in all cases? I had sort of assumed that we weren't doing that precisely because the document principal could differ from the "principal" we see there...

Comment on attachment 667379 [details] [diff] [review] Part 4 - Simplify calling doc logic in nsLocation. v1 Why is this doing GetDynamicScriptGlobal? It used to do GetStaticScriptGlobal. Why are we making that behavior change?

(In reply to Boris Zbarsky (:bz) from comment #12) > I guess this is fine, though this whole heuristic is kinda questionable in > various ways. If we're happy looking at the _document_ principal, why not > just go ahead and use the current URI of the document in all cases? I had > sort of assumed that we weren't doing that precisely because the document > principal could differ from the "principal" we see there... Well, this check is determining whether the document principal matches the document URI (which it doesn't for the JSURI cases, apparently). It's not clear to me what else this check would mean in the modern world.

(In reply to Boris Zbarsky (:bz) from comment #13) > Why is this doing GetDynamicScriptGlobal? It used to do > GetStaticScriptGlobal. Why are we making that behavior change? The former song and dance with GetCxSubjectPrincipalAndFrame was actually giving us the frame of the caller, meaning that the call to GetStaticScriptGlobal would give us the document in the immediate calling frame. So this does change behavior in the case that doc A calls into a function in doc B that sets location on doc C - previously, we'd use doc B for the URI check, and now we'll use doc A. But I'm pretty sure this change is spec correct.

(In reply to Bobby Holley (:bholley) from comment #15) > So this does change behavior in the case that doc A calls into a function in > doc B that sets location on doc C - previously, we'd use doc B for the URI > check, and now we'll use doc A. But I'm pretty sure this change is spec > correct. Or maybe not. The spec says that referrer must be the |Source Browsing Context|, defined as "the browsing context which was responsible for starting the navigation". This is a bit ambiguous, but the distinction is web-visible in the "a calls b which navigates c" case. My tests indicate the following: Firefox before my patch: referrer = b.html Firefox after my patch: referrer = a.html Webkit: referrer = b.html Opera: referrer = a.html The post-patch behavior is much nicer and more solid for us to implement, since we can implement it with the script entry point stack, as opposed to some kind of JS stack walking. But it seems like we should clear up the spec before proceeding here.

And in fact, clarifying the spec issue in comment 17 will answer Justin's question in comment 7. Is the referrer the script entry point, or the immediate caller?

Yep, comment 17 and comment 18 are spot on. What does IE do?

(In reply to Boris Zbarsky (:bz) from comment #19) > Yep, comment 17 and comment 18 are spot on. What does IE do? a.html. So Webkit and current firefox do b.html. Opera, IE, and post-patch Firefox do a.html.

"Lovely". I'm probably ok to switching to a.html if we can convince WebKit to....

(In reply to Boris Zbarsky (:bz) from comment #21) > "Lovely". > > I'm probably ok to switching to a.html if we can convince WebKit to.... Ok, should I file a spec bug?

I'd start by mailing the whatwg list, I guess, and explicitly asking the WebKit folks.

Comment on attachment 667379 [details] [diff] [review] Part 4 - Simplify calling doc logic in nsLocation. v1 I'm going to raise this as a spec issue. Cancelling the review for now.

This is an alternate approach that allows us to remove the API while maintaining our existing behavior with respect to document.referrer. Flagging bz for review.

Comment on attachment 669505 [details] [diff] [review] Part 3 - Use JS_GetScriptedCaller instead of JSStackFrames in nsLocation. v1 r=me. Was "principal" unused at this point? I guess it was...

Had a green try push in comment 1, but I've munged these patches a bit, so let's just be sure. https://tbpl.mozilla.org/?tree=Try&rev=7798c0611b22

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/238f3986fe71 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5760a66bfcf remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/798a316ebfaa remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/94317b3cb4d5

https://hg.mozilla.org/mozilla-central/rev/238f3986fe71 https://hg.mozilla.org/mozilla-central/rev/c5760a66bfcf https://hg.mozilla.org/mozilla-central/rev/798a316ebfaa https://hg.mozilla.org/mozilla-central/rev/94317b3cb4d5

Comment on attachment 669505 [details] [diff] [review] Part 3 - Use JS_GetScriptedCaller instead of JSStackFrames in nsLocation. v1 It looks like some of the ion-monkey stuff made this API pretty flakey and jit-dependent, so I think we should probably uplift this onto the branches. Flagging for approval. [Approval Request Comment] Bug caused by (feature/regressing bug #): IonMonkey and associated prep work User impact if declined: potential jit-dependent crashes. Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Low risk. Well-understood. String or UUID changes made by this patch: None.

(In reply to Bobby Holley (:bholley) from comment #30) > Comment on attachment 669505 [details] [diff] [review] > Part 3 - Use JS_GetScriptedCaller instead of JSStackFrames in nsLocation. v1 > > It looks like some of the ion-monkey stuff made this API pretty flakey and > jit-dependent, so I think we should probably uplift this onto the branches. > Flagging for approval. > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): IonMonkey and associated prep work > User impact if declined: potential jit-dependent crashes. Would we have seen a top crash around this already, or would the crashes have had varying signatures?

Comment on attachment 669505 [details] [diff] [review] Part 3 - Use JS_GetScriptedCaller instead of JSStackFrames in nsLocation. v1 Spoke with bholley - we'd be much more comfortable if we took this low-risk fix (not fixing this may cause unknown regressions). Thankfully it's early in the cycle, so let's take this.

Caught this too late for today's Beta 2 go to build, please get this landed asap so it will be in our next Beta.

(In reply to Lukas Blakk [:lsblakk] from comment #33) > Caught this too late for today's Beta 2 go to build, please get this landed > asap so it will be in our next Beta. After some discussion we're less sure that we want to take this on branch. I'm looking into it.

(In reply to Bobby Holley (:bholley) from comment #34) > After some discussion we're less sure that we want to take this on branch. > I'm looking into it. What is the result of your investigation? Are we going ahead with branch uplifts or not?

(In reply to Lukas Blakk [:lsblakk] from comment #35) > What is the result of your investigation? Are we going ahead with branch > uplifts or not? I think we should skip it for now.