Closed Bug 1071771 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Iteration:
35.3
Tracking Status
e10s m3+ ---

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Blocks: 1058717
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Flags: qe-verify?
Flags: firefox-backlog+
Hi Neil, can you provide a point value and mark the bug accordingly for verification.  Thanks.
Flags: needinfo?(enndeakin)
Points: --- → 5
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(enndeakin)
QA Contact: jbecerra
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.
Attached patch Workaround fix (obsolete) — Splinter Review
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)
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)
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+
Iteration: 35.2 → 35.3
Requesting re-triage now that we have a fix landed. Don't think this needs to remain in m3.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
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
Closed: 5 years ago5 years ago
You need to log in before you can comment on or make changes to this bug.