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

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Comment 1

[Marco Mucci [:MarcoM]]

Hi Neil, can you provide a point value and mark the bug accordingly for verification.  Thanks.

Comment 2

[Neil Deakin]

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.

Comment 3

[Neil Deakin]

Attachment: 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.

Comment 4

[Mike Conley (: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?

Comment 5

[Neil Deakin]

Attachment: 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.

Comment 6

[Mike Conley (: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?

Comment 7

[Neil Deakin]

Attachment: Workaround fix, v2.1

Don't think it hurts to have it in, but this patch takes it out again.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8495994 [details] [diff] [review]
Workaround fix, v2.1

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

LGTM, thanks Neil.

Comment 9

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34827dc36ee8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/34827dc36ee8

Comment 11

[Jim Mathies [:jimm]]

Requesting re-triage now that we have a fix landed. Don't think this needs to remain in m3.

Comment 12

[Jim Mathies [:jimm]]

Bug 1026099 addresses this issue as well.

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.