Closed
Bug 1164011
Opened 10 years ago
Closed 10 years ago
[e10s] regression: we're accidentally filtering out CPOWS in interposeProperty
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: gkrizsanits, Assigned: gkrizsanits)
Details
Attachments
(2 files, 1 obsolete file)
1.06 KB,
patch
|
billm
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
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.
Here are some tests. They currently fail.
Attachment #8604859 -
Flags: review?(mconley)
Updated•10 years ago
|
Comment 3•10 years ago
|
||
Comment on attachment 8604859 [details] [diff] [review]
patch
Review of attachment 8604859 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM, thanks billm.
Attachment #8604859 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
(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
Assignee | ||
Comment 7•10 years ago
|
||
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)
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+
https://hg.mozilla.org/mozilla-central/rev/f8993cea9f8e
https://hg.mozilla.org/mozilla-central/rev/63901344b6b0
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 11•10 years ago
|
||
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?
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
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 → ---
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Also extremely frequent WinXP debug crashes:
https://treeherder.mozilla.org/logviewer.html#?job_id=10275797&repo=mozilla-inbound
And I'm pretty sure these were yours as well:
https://treeherder.mozilla.org/logviewer.html#?job_id=10275970&repo=mozilla-inbound
Comment 16•10 years ago
|
||
(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.
Comment 17•10 years ago
|
||
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: 10 years ago → 10 years ago
Flags: needinfo?(gkrizsanits)
Resolution: --- → FIXED
Updated•10 years ago
|
status-firefox40:
--- → affected
Updated•10 years ago
|
Attachment #8605112 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
Please post hg links rather than treeherder links. Also, set the status flags when you land :)
Comment 20•10 years ago
|
||
Any reason the tests weren't also landed?
Flags: needinfo?(gkrizsanits)
Flags: in-testsuite+
Assignee | ||
Comment 21•10 years ago
|
||
(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)
Comment 22•10 years ago
|
||
Tests can land without additional approval.
You need to log in
before you can comment on or make changes to this bug.
Description
•