Closed Bug 1102025 Opened 5 years ago Closed 4 years ago

e10s - fix browser_keywordBookmarklets.js - javascript bookmarklet should inherit principal

Categories

(Firefox :: Bookmarks & History, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
e10s + ---
firefox45 --- fixed

People

(Reporter: Gijs, Assigned: mrbkap)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

So this test currently fails in e10s because this assertion:

ok(gBrowser.contentPrincipal.equals(originalPrincipal), "javascript bookmarklet should inherit principal");

is false.

Looking at this, it seems under e10s both principals are null principals (but with different IDs), whereas under non-e10s, both are about:blank.

Dao, should this just be waiting for about:blank to load, or is something else going on? And could this potentially cause security issues? (CC'ing bholley and bz just in case they want to chime in here)
Flags: qe-verify-
Flags: needinfo?(dao)
Flags: in-testsuite+
Flags: firefox-backlog+
(In reply to :Gijs Kruitbosch from comment #0)
> Dao, should this just be waiting for about:blank to load,

It already waits for the pageshow event.

> or is something
> else going on? And could this potentially cause security issues?

I have no idea.
Flags: needinfo?(dao)
So it seems like the way we set up contentPrincipal is totally different with e10s:

normally, we just pull it off the content document: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#452

with e10s, it's some sort of static property:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#141

And the only place I can find it being set is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/RemoteWebProgress.jsm#188

Which looks fishy.
Attached patch Proposed fix (obsolete) — Splinter Review
In e10s, null principals can be serialized across processes, but identity isn't maintained, so given some hypothetical code:

== parent ==

addMessageListener("message1", (msg) => { principal1 = msg.data.principal; });
addMessageListener("message2", (msg) => { dump(msg.data.principal === principal1); });

== child ==

let principal = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);
sendAsyncMessage("message1", { principal });
sendAsyncMessage("message2", { principal });

we'll dump false in e10s. So, even though the principal should be the same in the content process, the test fails.
Attachment #8696145 - Flags: review?(bobbyholley)
Assignee: nobody → mrbkap
Comment on attachment 8696145 [details] [diff] [review]
Proposed fix

Review of attachment 8696145 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming that we don't in fact need to manage the identity...
Attachment #8696145 - Flags: review?(bobbyholley) → review+
I realized before checking this in that we can have our cake and eat it too in e10s.
Attachment #8697050 - Flags: review?(bobbyholley)
Attachment #8696145 - Attachment is obsolete: true
Attachment #8697050 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/98b0b0d51cec
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.