Issue 29723684: Issue 6382 - Catch InvalidAccessError exception when checking styles. (Closed)
|
Created:
March 15, 2018, 9:43 p.m. by hub Modified:
April 19, 2018, 11:56 a.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6382 - Catch InvalidAccessError exception when checking styles.
If the exception isn't caught, the filtering will never happen.
Patch Set 1 #
Total comments: 6
Patch Set 2 : Shrink the try{} block #
Total comments: 3
Patch Set 3 : Link to the code to explain the exception #MessagesTotal messages: 7
https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... lib/content/elemHideEmulation.js:531: let rules = stylesheet.cssRules; If this is the only line that could cause the exception to be thrown, perhaps we should only wrap this line in the try? https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... lib/content/elemHideEmulation.js:545: // On Firefox, there is a chance that an InvalidAccessError How come this happens? Is there a Firefox bug / did you file one?
https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... lib/content/elemHideEmulation.js:531: let rules = stylesheet.cssRules; On 2018/03/19 15:52:07, kzar wrote: > If this is the only line that could cause the exception to be thrown, perhaps we > should only wrap this line in the try? Done.
https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... lib/content/elemHideEmulation.js:545: // On Firefox, there is a chance that an InvalidAccessError On 2018/03/19 15:52:07, kzar wrote: > How come this happens? Is there a Firefox bug / did you file one? It's not a bug. https://searchfox.org/mozilla-central/source/layout/style/StyleSheet.cpp#732 The stylesheet haven't been completely loaded yet. AreRulesAvailable() is called when GetCssRules() is called. I'm not sure if there is anything we can do to ensure rules are available, but catching the exception allow to not cancel the processing for the rest.
Sorry again for being so slow with these reviews. https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... lib/content/elemHideEmulation.js:545: // On Firefox, there is a chance that an InvalidAccessError On 2018/03/20 16:16:02, hub wrote: > On 2018/03/19 15:52:07, kzar wrote: > > How come this happens? Is there a Firefox bug / did you file one? > > It's not a bug. > > https://searchfox.org/mozilla-central/source/layout/style/StyleSheet.cpp#732 > > The stylesheet haven't been completely loaded yet. AreRulesAvailable() is called > when GetCssRules() is called. > > I'm not sure if there is anything we can do to ensure rules are available, but > catching the exception allow to not cancel the processing for the rest. Acknowledged. In fact that link you gave is pretty useful, perhaps add that instead of the link to our issue tracker? (Well a permalink for the right line like https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388f... ) https://codereview.adblockplus.org/29723684/diff/29728600/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29728600/lib/content/elemHid... lib/content/elemHideEmulation.js:622: continue; I wonder if we should check that the exception really is an InvalidAccessError before continuing? Then we wouldn't ignore other exceptions, also we could shorten the comment since the code would already be clear about the type of exception we're worrying about.
https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29723685/lib/content/elemHid... lib/content/elemHideEmulation.js:545: // On Firefox, there is a chance that an InvalidAccessError On 2018/04/18 16:12:30, kzar wrote: > On 2018/03/20 16:16:02, hub wrote: > > On 2018/03/19 15:52:07, kzar wrote: > > > How come this happens? Is there a Firefox bug / did you file one? > > > > It's not a bug. > > > > https://searchfox.org/mozilla-central/source/layout/style/StyleSheet.cpp#732 > > > > The stylesheet haven't been completely loaded yet. AreRulesAvailable() is > called > > when GetCssRules() is called. > > > > I'm not sure if there is anything we can do to ensure rules are available, but > > catching the exception allow to not cancel the processing for the rest. > > Acknowledged. > > In fact that link you gave is pretty useful, perhaps add that instead of the > link to our issue tracker? (Well a permalink for the right line like > https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388f... > ) Done. https://codereview.adblockplus.org/29723684/diff/29728600/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29728600/lib/content/elemHid... lib/content/elemHideEmulation.js:622: continue; On 2018/04/18 16:12:30, kzar wrote: > I wonder if we should check that the exception really is an InvalidAccessError > before continuing? Then we wouldn't ignore other exceptions, also we could > shorten the comment since the code would already be clear about the type of > exception we're worrying about. The intent here is to catch the exception to not interrupt the processing. There is nothing much we can do instead so I don't think it matters.
LGTM https://codereview.adblockplus.org/29723684/diff/29728600/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29723684/diff/29728600/lib/content/elemHid... lib/content/elemHideEmulation.js:622: continue; On 2018/04/18 18:48:23, hub wrote: > On 2018/04/18 16:12:30, kzar wrote: > > I wonder if we should check that the exception really is an InvalidAccessError > > before continuing? Then we wouldn't ignore other exceptions, also we could > > shorten the comment since the code would already be clear about the type of > > exception we're worrying about. > > The intent here is to catch the exception to not interrupt the processing. There > is nothing much we can do instead so I don't think it matters. Acknowledged. 
|
