[e10s] Spellchecker context menu initialization fails with Permission denied to access property 'value'

RESOLVED FIXED

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

unspecified
Points:
5
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10sm3+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Blocks: 1058717

Updated

4 years ago
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Flags: qe-verify?
Flags: firefox-backlog+
tracking-e10s: --- → m3+
Hi Neil, can you provide a point value and mark the bug accordingly for verification.  Thanks.
Flags: needinfo?(enndeakin)
(Assignee)

Updated

4 years ago
Points: --- → 5
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(enndeakin)

Updated

4 years ago
QA Contact: jbecerra
(Assignee)

Comment 2

4 years ago
So the issue here is that GetDictionaryList is being called on a content process spellchecker object, and it fails to set the out-parameters properly.
(Assignee)

Comment 3

4 years ago
Created attachment 8494637 [details] [diff] [review]
Workaround fix

This patch works around the issue by adding error checking to prevent the context menu from failing to initialize.

This doesn't fix the issue and just disables the dictionary list in the context menu, but I think that this may be the cause of 5-7 of the bugs marked as dependencies of bug 1060070 and this patch is a clear improvement over the current situation, until a better fix is available.
Attachment #8494637 - Flags: review?(mconley)
Comment on attachment 8494637 [details] [diff] [review]
Workaround fix

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

::: toolkit/modules/InlineSpellChecker.jsm
@@ +164,5 @@
> +      var o1 = {}, o2 = {};
> +      spellchecker.GetDictionaryList(o1, o2);
> +      list = o1.value;
> +      listcount = o2.value;
> +    } catch(e) { return 0 }

Instead of wrapping in a try/catch block like this, can we go through the menu to the browser window and check if gMultiProcessBrowser is true, and return 0 in that case?
Attachment #8494637 - Flags: review?(mconley)
(Assignee)

Comment 5

4 years ago
Created attachment 8495312 [details] [diff] [review]
Workaround fix, v2

I don't think we should be adding a dependency on a browser-only field here, but I can check for a spellchecker being inaccesible.
Attachment #8494637 - Attachment is obsolete: true
Attachment #8495312 - Flags: review?(mconley)
Comment on attachment 8495312 [details] [diff] [review]
Workaround fix, v2

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

::: toolkit/modules/InlineSpellChecker.jsm
@@ +164,5 @@
> +    if (Components.utils.isCrossProcessWrapper(spellchecker))
> +      return 0;
> +
> +    let list, listcount;
> +    try {

With the CPOW check, I don't think it's necessary to do our big try-catch anymore, is it?
Attachment #8495312 - Flags: review?(mconley)
(Assignee)

Comment 7

4 years ago
Created attachment 8495994 [details] [diff] [review]
Workaround fix, v2.1

Don't think it hurts to have it in, but this patch takes it out again.
Attachment #8495994 - Flags: review?(mconley)
Comment on attachment 8495994 [details] [diff] [review]
Workaround fix, v2.1

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

LGTM, thanks Neil.
Attachment #8495994 - Flags: review?(mconley) → review+

Updated

4 years ago
Iteration: 35.2 → 35.3

Comment 11

4 years ago
Requesting re-triage now that we have a fix landed. Don't think this needs to remain in m3.
tracking-e10s: m3+ → ?
tracking-e10s: ? → m3+

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED

Comment 12

4 years ago
Bug 1026099 addresses this issue as well.
Setting as qe-verify-, since it appears that this was already (at least partially) verified by Alice0775 White in bug 1058717. Also I did a quick sanity check in the latest Firefox 36 Nightly (BuildID=	20141102030204) and there don't seem to be any obvious issues with the context menu (except maybe for the problems tracked in bug 1060070). 

If anyone considers that more extensive manual testing is needed, please set the flag back and specify what you think would need to be covered.
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → qe-verify-
Status: VERIFIED → RESOLVED
Last Resolved: 4 years ago4 years ago
You need to log in before you can comment on or make changes to this bug.