Closed Bug 1164011 Opened 6 years ago Closed 6 years ago

[e10s] regression: we're accidentally filtering out CPOWS in interposeProperty

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
e10s m7+ ---
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

Details

Attachments

(2 files, 1 obsolete file)

I talked about this with Bill, I have a patch for it in my queue, I will file it together with the shim optimization patches. I think this should be lifted up to aurora. And this should be m7.
Assignee: nobody → gkrizsanits
tracking-e10s: --- → ?
We also have a couple shims that operate on CPOWs (ContentDocShellTreeItem and ContentDocument). This regressed those, as well as prefetching. Unfortunately we don't have any tests for those shims. This is probably a good time to add them.
Attached patch patch (obsolete) — Splinter Review
Here are some tests. They currently fail.
Attachment #8604859 - Flags: review?(mconley)
Comment on attachment 8604859 [details] [diff] [review]
patch

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

LGTM, thanks billm.
Attachment #8604859 - Flags: review?(mconley) → review+
Thanks for tests Bill! Saved me tons of time with them...
Attachment #8605112 - Flags: review?(wmccloskey)
Comment on attachment 8605112 [details] [diff] [review]
interposition for CPOWS

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

Thanks, sorry for the delay. Can you check in both patches when you're ready?
Attachment #8605112 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #5)
> Can you check in both patches when you're ready?

It turns out the test does not work without the --e10s flag :(

52 INFO TEST-UNEXPECTED-FAIL | toolkit/components/addoncompat/tests/browser/browser_addonShims.js | got expected import result - Got [object XULElement], expected [object XULElement]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89cd574587fc
Is there a way to run that part of the test in e10s mode only? I'm not quite sure how should I fix that test, I might land the patch without it, and land the test later... the fix should be uplifted to aurora ASAP I think.
Flags: needinfo?(wmccloskey)
Attached patch tests v2Splinter Review
Here's a new version of the test that works without --e10s.
Attachment #8604859 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Attachment #8609053 - Flags: review+
Comment on attachment 8605112 [details] [diff] [review]
interposition for CPOWS

I think this should be uplifted.

Approval Request Comment
[Feature/regressing bug #]: Bug 1101182
[User impact if declined]: Various add-on breakage in e10s mode (some part of the shim layer is broken)
[Describe test coverage new/current, TreeHerder]: There is a test attached to the bug, it's doing fine on mc currently.
[Risks and why]: I see no risk, simple and straightforward patch that gains a lot in terms of user experience for e10s users.
[String/UUID change made/needed]: none
Attachment #8605112 - Flags: approval-mozilla-aurora?
Backed out for frequent xpc::InterposeProperty crashes. And did this landed under the correct bug number?
https://treeherder.mozilla.org/logviewer.html#?job_id=10268935&repo=mozilla-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(gkrizsanits)
Resolution: FIXED → ---
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> Backed out for frequent xpc::InterposeProperty crashes. And did this landed
> under the correct bug number?

It did not, these were for bug 1164014.
Setting this bug as Resolved Fixed per #c16. The discussion should move back to bug 1164014.
(Also clearing the needinfo here and instead moving it to bug 1164014.)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(gkrizsanits)
Resolution: --- → FIXED
Attachment #8605112 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please post hg links rather than treeherder links. Also, set the status flags when you land :)
Any reason the tests weren't also landed?
Flags: needinfo?(gkrizsanits)
Flags: in-testsuite+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> Please post hg links rather than treeherder links. Also, set the status
> flags when you land :)

alright: https://hg.mozilla.org/releases/mozilla-aurora/rev/12b3775fe29f

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> Any reason the tests weren't also landed?

Hmm... I think the test should work as well on aurora. I was not entirely
sure if it uses anything that we fixed on mc but was not uplifted, but now that
I'm thinking about it, it should not be the case.
Do you think we need to file another aurora request for that or is it OK to just
land the test as well?
Flags: needinfo?(gkrizsanits)
Tests can land without additional approval.
You need to log in before you can comment on or make changes to this bug.